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.

Sign up for GitHub

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

Jump to bottom

Offline model building: system_structure and related functionality #542

Merged
merged 57 commits into from
Mar 31, 2020
Merged

Offline model building: system_structure and related functionality #542

merged 57 commits into from
Mar 31, 2020

Conversation

kyllingstad
Copy link
Member

The main thing in this PR is the addition of system_structure, along with related functions and classes. This fixes #114.

system_structure is a class that holds more or less the same information as an OspSystemStructure.xml or SystemStructure.ssd:

The whole point is to be able to:

  1. Represent the system structure "in memory" so it can be reused for several executions, possibly with different initial values. (Initial values are kept in a separate variable_value_map which is not part of the system_structure, but which is also validated against it.)
  2. Provide a unified mechanism for constructing systems from named entities and validating them. This is done in two different ways in the CSE config and SSP parsers now.

What I have not done in this PR is to rewrite the CSE and SSP parsers in terms of system_structure. This is mainly to keep this PR focused and prevent it from growing entirely out of proportions. I will update the parsers in an upcoming PR based on this one. When that is done, the parsers should only contain parsing code, and thus be a lot cleaner.

P.S. This is a revamp of an earlier PR, #427.

@kyllingstad
WIP on system_structure
812db2c
@kyllingstad
More WIP on system_structure
e18df88
@kyllingstad
Even more WIP on system_structure
ab5721f
@kyllingstad
WIP
a05a40d
@kyllingstad
Move visitor to utility header
c7e3743
@kyllingstad
Merge remote-tracking branch 'origin/feature/split-off-visitor' into …
654d481 
…feature/114-offline-model-building
@kyllingstad
WIP: Initial values
fec0f81
@kyllingstad
WIP: Add docs and update test
5283416
@kyllingstad
Fix typo
13b1c1f
@kyllingstad
Remove half-written code in remote_fmu.cpp
17d8581
@kyllingstad
Merge remote-tracking branch 'origin/master' into feature/114-offline…
b12ce6d 
…-model-building
@kyllingstad
Improve SSP parser "default experiment" handling
8d11273
@kyllingstad
Handle both SSP dir and filename
1d85243
@kyllingstad
Update other SSP tests too
83a9b6d
@kyllingstad
Drop source() from cse::model again
99fb9ff
@kyllingstad
Merge remote-tracking branch 'origin/master' into feature/114-offline…
08acc94 
…-model-building
@kyllingstad
Fix load_ssp() documentation
2ed0880
@kyllingstad
Document simulator name rule, improve error msg.
e02f445
@kyllingstad
Use ':' to separate simulator and variable name
49bceed
@kyllingstad
Rename variable_qname to full_variable_name
87157fd
@kyllingstad
If-with-initialiser FTW
f077da0
@kyllingstad
Merge remote-tracking branch 'origin/master' into feature/114-offline…
282e063 
…-model-building
@kyllingstad
Add function API and example function
377c23b
@kyllingstad
WIP
9ce9695
@kyllingstad
Complete(?) function API
9db2b1a
@kyllingstad
Remove connections
cc884d8
@kyllingstad
Minor fixes here and there, update tests
3393996
@kyllingstad
NOMINMAX again
096571e
@kyllingstad
Add function unittests
5a17639
@kyllingstad
Add forgotten function_unittests
cd66c82
@kyllingstad
Fix mock_slave and lots of magic numbers
dc0c527 
This fixes issue #538, "`mock_slave` violates assumptions, causing
tests to be ill-defined".  Basically, what I've done is to make the
user-supplied functions act on the outputs rather than on the internal
states.

This had quite substantial effects on some of the tests, which depended
on the wrong behaviour.  To compensate and, to some extent, restore
the old behaviour in a "correct" way, I've had to add some new
functionality to `mock_slave`, namely:

- The possibility to have outputs depend on time as well as inputs.
- The possibility to have a custom function called at each time step.

Finally, I've taken this opportunity to make some of the tests slightly
more readable by replacing magic numbers with named variables/constants.
This includes:

- Simulator indexes, which were assumed to be sequential in some tests
- Variable references, which were assumed to be 0 and 1 for
  `mock_slave`'s variables

The magic numbers were making the tests hard to read, and therefore also
to fix.  Besides, while the above assumptions are correct at the moment,
that need not be the case in the future.
@kyllingstad
Merge branch bugfix/538-... and complete tests
5cd2311
@kyllingstad
Fix MSVC warnings
6dad16e
@kyllingstad
Merge branch 'master' into feature/114-offline-model-building
dcb68ca
@kyllingstad
Merge branch 'feature/464-connection-introspection' into feature/114-…
870bfd9 
…offline-model-building
@kyllingstad
Rename scalar_connection to connection
f1a4d93
@kyllingstad
Use "entity" instead of "simulator"
5e1702c
@kyllingstad
Define alias for function parameter maps
0232093
@kyllingstad
Public enums for function parameter indexes
b9d50a7
@kyllingstad
Complete support for functions in system_structure
f79c0a5
@kyllingstad
Merge branch 'feature/464-connection-introspection' into feature/114-…
136be4b 
…offline-model-building
@kyllingstad
Move implementation of to_text() to cpp file
4f13557
@kyllingstad kyllingstad added the enhancement New feature or request label Mar 23, 2020
@kyllingstad kyllingstad self-assigned this Mar 23, 2020
@markaren
Copy link
Contributor

With a system_structure in place, I hope we can return this object from the parser rather than an execution object. The execution should take a system_structure as input.

@markaren
Copy link
Contributor

Looking at the PR though, I guess inject_system_structure might be sufficient rather then require the ctor to handle it. Perhaps you had the same idea with the parsers @kyllingstad ?

@kyllingstad
Copy link
Member Author

With a system_structure in place, I hope we can return this object from the parser rather than an execution object.

Yes! :) The plan is for the parsers to return a system_structure along with a list of variable_value_map objects that hold the parameter sets.

The execution should take a system_structure as input.

Looking at the PR though, I guess inject_system_structure might be sufficient rather then require the ctor to handle it. Perhaps you had the same idea with the parsers @kyllingstad ?

I considered adding an execution ctor that takes a system_structure, but figured that the current solution with inject_system_structure() would give more flexibility without being much more verbose. A convenience ctor that takes a system_structure can of course be added later if we see the need. It can be trivially implemented in terms of inject_system_structure()

@kyllingstad kyllingstad force-pushed the feature/114-offline-model-building branch from d59806c to 4f13557 Compare March 25, 2020 06:51
@kyllingstad kyllingstad mentioned this pull request Mar 27, 2020
Merged
@kyllingstad kyllingstad changed the base branch from feature/464-connection-introspection to master March 27, 2020 14:06
hplatou
hplatou approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@hplatou hplatou left a comment

Choose a reason for hiding this comment

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

Very nice!🥇

@kyllingstad kyllingstad merged commit ded0150 into master Mar 31, 2020
@kyllingstad kyllingstad deleted the feature/114-offline-model-building branch March 31, 2020 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hplatou hplatou hplatou approved these changes

@ljamt ljamt Awaiting requested review from ljamt

@eidekrist eidekrist Awaiting requested review from eidekrist

Assignees

@kyllingstad kyllingstad

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add functionality for offline model building
3 participants
@kyllingstad @markaren @hplatou