Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor/ssp-move-algorithm-to-DefaultExperiment #447

Merged
merged 11 commits into from
Nov 4, 2019

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Oct 30, 2019

This PR moves the osp runtime annotation stuff inside DefaultExperiment where it belongs.
This also reduces the complexity of ssp_parser.cpp

I have also renamed osp:SimulationInformation to osp:Algorithm and removed the description tag.
osp:FixedStepMaster is renamed to osp:FixedStepAlgorithm

Old:

<ssd:Annotations>
      <ssc:Annotation type="org.open-simulation-platform">
        <osp:SimulationInformation>
          <osp:FixedStepMaster description="FixedStepAlgorithm" stepSize="1e-4"/>
        </osp:SimulationInformation>
      </ssc:Annotation>
  </ssd:Annotations>

New:

<ssd:DefaultExperiment>
        <ssd:Annotations>
            <ssc:Annotation type="com.opensimulationplatform">
                <osp:Algorithm>
                    <osp:FixedStepAlgorithm stepSize="1e-4"/>
                </osp:Algorithm>
            </ssc:Annotation>
        </ssd:Annotations>
  </ssd:DefaultExperiment>

It also adds a xsd.

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for leaving this hanging for so long. Mostly good, just some small change requests from my side. Also, I'd like a second opinion on the XSD change.

src/cpp/ssp_parser.cpp Outdated Show resolved Hide resolved
src/cpp/ssp_parser.cpp Outdated Show resolved Hide resolved
test/data/ssp/OSPAnnotations.xsd Show resolved Hide resolved
src/cpp/ssp_parser.cpp Outdated Show resolved Hide resolved
Copy link
Member

@eidekrist eidekrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the Jenkins build (can be fixed by merging master into this) things look good from my point of view.

@markaren
Copy link
Contributor Author

markaren commented Oct 31, 2019

How about stepSize vs baseStepSize for fixedStep?

Copy link
Member

@kyllingstad kyllingstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now.

@markaren
Copy link
Contributor Author

markaren commented Nov 1, 2019

I'd like to get a verdict on stepSize vs baseStepSize for the osp:FixedStepAlgoritm element before finally merging this PR. IMO baseStepSize makes more sense.

@markaren
Copy link
Contributor Author

markaren commented Nov 4, 2019

If there is no objections by the end of the day I'll merge using baseStepSize

@kyllingstad
Copy link
Member

I agree, baseStepSize has a clearer meaning than stepSize.

@markaren markaren merged commit 75fb207 into master Nov 4, 2019
@markaren markaren deleted the refactor/ssp-move-algo-to-defaultexperiment branch November 5, 2019 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants