open-simulation-platform / libcosim

OSP C++ co-simulation library
https://open-simulation-platform.github.io/libcosim
Mozilla Public License 2.0
61 stars 10 forks source link

Separate variable reference, type, causality into separate header lines #725

Closed restenb closed 1 year ago

restenb commented 1 year ago

This PR proposes to change the variable_name [ref, type, causality] pattern in the current file logging implementation to one new line for each. This might be easier to use when parsing the CSV files.

In other words the values will now be logged starting from line 5, and the new CSV files look like this:

  1. Time,StepCount,realOut,intOut,booleanOut,stringOut
  2. ,#,0,0,0,0

  3. ,#,real,integer,boolean,string

  4. ,#,output,output,output,output

  5. 0,0,1.234,1,0,hello log
  6. 0.1,1,1.234,1,0,hello log
markaren commented 1 year ago

Not sure I agree on this one. realOut [ref type casualty] is easy enough to parse, even without regex. I also think some parsers like pandas in python asumes a header on the first line followed by data?

I don't really see the problem to be solved here. #712 raised an issue related to ref which could change over time. That one can be solved in libcosim by removing the ref identifier as it is redundant anyway.

kyllingstad commented 1 year ago

Not sure I agree on this one.

Me neither. It's kind of a de facto standard for CSV to have at most one header line. Not all CSV parsers will support skipping, let alone parsing, multiple header lines. I think we should simply drop the metadata from the header (as I also just commented in #712).

restenb commented 1 year ago

Good input! I originally added [ref, type, causality] after the variable name just to somehow squeeze all information on one line, but it always felt awkward. Most ready-to-use plotting tools probably assume it's just the name of the signal/axis as well, and it might be hard for such users to really do anything about it if they don't want [0, real, input] in their presentation plots.

So I have removed the extra header lines again for now. We can provide the other information in a separate metadata file perhaps?

kyllingstad commented 1 year ago

Yeah. cosim provides the information in machine-readable form (YAML, specifically), but only for individual FMUs. You can see an example under cosim inspect here: https://open-simulation-platform.github.io/cosim

restenb commented 1 year ago

I looked at the implementation of cosim inspect and decided to bring it over to the file logger as well, so now this branch will output a logFileName_metadata.yaml file alongside the CSV, with the contents exactly the same as what cosim inspect puts on stdout.

You can have a look with file_observer_logging_test or any of the other tests that log to file. If anybody is parsing the old format it'll be a breaking change of course, but I think this is a better way of separating the simulation results from it's metadata.

I'll be looking to release libcosim in the coming days - hopefully this week - so we need to decide on whether this is to be included in that as well.