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

Issue 1976 do not log pointless time variables #2002

Merged
merged 7 commits into from
Nov 1, 2020
Merged

Issue 1976 do not log pointless time variables #2002

merged 7 commits into from
Nov 1, 2020

Conversation

peterNordin
Copy link
Member

@peterNordin peterNordin commented Sep 25, 2020 •
edited
Loading

  • Unify CSV and HDF5 output code in HopsanCLI to make them behave the same way
  • Both support logonly include filter
  • Both support only exporting relevant time vectors (depending on variables exported),
    Example: If only a subsystem variable is exported, then only the time vector for that system is
  • If simulation is aborted, then no longer export the entire pre-allocated time vector, only up to the last actual sample
  • Reserved the names Time, time, Frequency, frequency, to avoid name collission in HDF5 exporter, HopsanGUI will autorename components and system parameter when loading a model. HopsanCLI will not, and models must be fixed manually.
  • Various fixes and code cleanup in hdf5exporter

Issues still needing to be solved:

  • Name collision between component named Time and Time vector of a system

Related to #1976

Test

Assuming debug build!

Run this before updating to this code:

./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFullCSV beforeFullCSV.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFinalCSV beforeFinalCSV.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFullHDF5 beforeFullHDF5.h5
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFinalHDF5 beforeFinalHDF5.h5

./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFullCSV beforeFullCSVonly.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFinalCSV beforeFinalCSVonly.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFinalHDF5 beforeFinalHDF5only.h5
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFullHDF5 beforeFullHDF5only.h5

./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Subsubsystem#SubsubPortOut" --resultsFullCSV beforeFullCSVonlysysport.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Subsubsystem#SubsubPortOut" --resultsFullHDF5 beforeFullHDF5onlysysport.h5

Run this after updating to this code:

./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFullCSV afterFullCSV.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFinalCSV afterFinalCSV.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFullHDF5 afterFullHDF5.h5
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --resultsFinalHDF5 afterFinalHDF5.h5

./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFullCSV afterFullCSVonly.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFinalCSV afterFinalCSVonly.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFinalHDF5 afterFinalHDF5only.h5
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Gain#out" --resultsFullHDF5 afterFullHDF5only.h5

./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Subsubsystem#SubsubPortOut" --resultsFullCSV afterFullCSVonlysysport.csv
./hopsancli_d -m ../UnitTests/HopsanCoreTests/SimulationTest/unittestmodel.hmf -s hmf --logonly "Subsystem\$Subsubsystem#SubsubPortOut" --resultsFullHDF5 afterFullHDF5onlysysport.h5

Run the following to see the differences:

diff -u beforeFinalCSV.csv afterFinalCSV.csv | cut -d, -f1   # this one no longer includes start values for input variables
diff -u beforeFullCSV.csv afterFullCSV.csv | cut -d, -f1

diff -u beforeFinalCSVonly.csv afterFinalCSVonly.csv | cut -d, -f1
diff -u beforeFullCSVonly.csv afterFullCSVonly.csv | cut -d, -f1

diff -u beforeFullCSVonlysysport.csv afterFullCSVonlysysport.csv | cut -d, -f1

../dependencies/hdf5/bin/h5diff -v beforeFinalHDF5.h5 afterFinalHDF5.h5
../dependencies/hdf5/bin/h5diff -v beforeFullHDF5.h5 afterFullHDF5.h5

../dependencies/hdf5/bin/h5diff -v beforeFinalHDF5only.h5 afterFinalHDF5only.h5
../dependencies/hdf5/bin/h5diff -v beforeFullHDF5only.h5 afterFullHDF5only.h5

../dependencies/hdf5/bin/h5diff -v  beforeFullHDF5onlysysport.h5 afterFullHDF5onlysysport.h5

@peterNordin peterNordin marked this pull request as ready for review October 5, 2020 18:53
robbr48
robbr48 approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@robbr48 robbr48 left a comment

Choose a reason for hiding this comment

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

Seems to work! It is unfortunate that we have a component with the default display name "Time". It should probably be changed to "SimulationTime" or similar. Then we can make "Time" a reserved name. Only problem is of course the backward compatibility.

@peterNordin peterNordin force-pushed the issue-1976_do_not_log_pointless_time_variables branch from a71b56b to d4f6ffa Compare October 8, 2020 19:03
@peterNordin
Copy link
Member Author

@robbr48 Can you look over the last two commits as well, I reserved Time, time, Frequency, frequency and attempt to autoupdate old models (only in HopsanGUI).

Things like aliases probably wont update, and users of system variables with these names will need to be updated manually.

@peterNordin
Unifying CSV and HDF5 export code to make them behave the same way
e87cbd9
@peterNordin
Add test for data export algorithm
a1bc51c
@peterNordin
Various improvements to hdf5 exporter code
b94e2e9 
- Code cleanup and clarification
- In the event of dataset creation failure, remaining data will
still be written
- Print error message in CLI if hdf5 export has errors
@peterNordin
Reserve names Time, time, Frequency and frequency
12eb646 
Try to rename component and system parameters when loading model in GUI
@peterNordin
Change default name of SignalTime to SimulationTime
8d631f6 
Also rename files to match source code file
@peterNordin
Ensure only logged data samples are exported
ba30c22 
When simulation is aborted, time and data vectors will be fully allocated
but data not written is junk. Only export up until last logged sample.
@peterNordin
Remove obsolete saveResults function
5908e7d
@peterNordin peterNordin force-pushed the issue-1976_do_not_log_pointless_time_variables branch from d4f6ffa to 5908e7d Compare October 20, 2020 19:47
@peterNordin peterNordin mentioned this pull request Oct 20, 2020
Open
@peterNordin peterNordin added this to the 2.16.0 milestone Nov 1, 2020
@peterNordin peterNordin merged commit 5908e7d into Hopsan:master Nov 1, 2020
@peterNordin peterNordin deleted the issue-1976_do_not_log_pointless_time_variables branch December 8, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robbr48 robbr48 Awaiting requested review from robbr48

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.16.0
Development

Successfully merging this pull request may close these issues.
2 participants
@peterNordin @robbr48