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

[SSP] Move and rename osp algorithm annotation into DefaultExperiment & make algorithms pluggable #426

Closed
wants to merge 14 commits into from

Conversation

markaren
Copy link
Contributor

@markaren markaren commented Oct 16, 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="com.opensimulationplatform">
        <osp:SimulationInformation>
          <osp:FixedStepMaster description="FixedStepAlgorithm" stepSize="1e-4"/>
        </osp:SimulationInformation>
      </ssc:Annotation>
  </ssd:Annotations>

New:

<ssd:DefaultExperiment startTime="0.0">
        <ssd:Annotations>
            <ssc:Annotation type="com.opensimulationplatform">
                <osp:Algorithm>
                    <osp:FixedStepAlgorithm baseStepSize="1e-4">
                       <osp:StepSizes>
                           <osp:StepSize component="CraneController" multipleOf="2"/>
                       </osp:StepSizes>
                </osp:Algorithm>
            </ssc:Annotation>
        </ssd:Annotations>
  </ssd:DefaultExperiment>

This PR might be hard to swallow since it is non backwards compatible. But IMO it is a great improvement.

Closes #435
Closes #437

@markaren markaren changed the title Move and rename osp algorithm annotation into DefaultExperiment [SSP] Move and rename osp algorithm annotation into DefaultExperiment Oct 16, 2019
@markaren
Copy link
Contributor Author

Side note:
What I like about

<osp:Algorithm>
    <osp:FixedStepAlgorithm stepSize="1e-4"/>
</osp:Algorithm>

Is that it is extensible and allows custom properties for each Algorithm. The new OspSystemStructure does not..
However, we will need to implement a system to parse the element found within osp:Algorithm similar to how we do it for simulators within the SSP itself.

@kyllingstad
Copy link
Member

I like this! Not only the XML change, but also that it combines the SimulationInformation and DefaultExperiment types in code. Personally, I don't think we should worry too much about backwards compatibility at the pre-1.0 stages, but maybe @ljamt should be the one to approve this one since it will affect some/all of the work in WP4.

@markaren
Copy link
Contributor Author

I have added a plugin based system for parsing algorithms, modelled after the model_uri_resolver.
It may or may not fit this PR, but the alternative was a PR targeting this branch again.

@markaren
Copy link
Contributor Author

This is a first draft. I want feedback. E.g. this API requires the user to pass a default algorithm_resolver object to ssp_parser even though overrideAlgorithm is non-null. Also, how can we handle applying decimationFactors using this API?

@markaren
Copy link
Contributor Author

markaren commented Oct 22, 2019

Preferably I think load_ssp should be wrapped in a statefull class.

auto ssp_loader = cse::ssp_loader();
ssp_loader .set_override_algorithm = ...
ssp_loader .load(xmlPath);

@markaren markaren changed the title [SSP] Move and rename osp algorithm annotation into DefaultExperiment [SSP] Move and rename osp algorithm annotation into DefaultExperiment & make algorithms pluggable Oct 22, 2019
@markaren
Copy link
Contributor Author

markaren commented Oct 23, 2019

This PR is a huge improvement to the code base.

  1. The SSP XML looks better and fits better with the spec. Also the resulting code handling is simplified.
  2. Loading ssp is much easier since you now only need to specify what you actually need.
  3. The implementation is 100% algorithm agnostic!

But some cleanup/moving of files is surely needed.

@markaren
Copy link
Contributor Author

Added support for multiple-step-sizes with the help of #439

@kyllingstad
Copy link
Member

I have added a plugin based system for parsing algorithms, modelled after the model_uri_resolver.
It may or may not fit this PR,

Personally, I'd prefer it if they were two separate PRs. The latest "plugability" change seems to be independent of the XML change, and at least in my mind, it's quite a bit more controversial.

but the alternative was a PR targeting this branch again.

That's actually not a bad alternative. The second PR can always be retargeted on master when the first one has been merged.

@markaren
Copy link
Contributor Author

I can make it into two separate PRs tomorrow.

@markaren
Copy link
Contributor Author

Closed in favor of #447, #448 and #449

@markaren markaren closed this Oct 30, 2019
@markaren markaren deleted the move-ssp-annotations branch November 1, 2019 11:58
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.

[SSP] Support any algorithm [SSP] Support multi-step-size configuration
2 participants