Skip to content

Conversation

@arsLibera
Copy link
Contributor

Add support for Json format input files

Current situation

We have discussed adding support for JSON input files here:
Add support for JSON input files

Currently, we have unit tests for the existing legacy format *.in to lock down that behavior, and system tests that also use *.in input files.

Release Notes

This change adds support for a V0 for JSON input files and associated unit tests.
The legacy behavior of the existing executable program is preserved: a single input argument will be interpreted as a legacy input *.in file, and execute as before. If input argument flags are provided, the new behavior will be utilized as per the description in main.cxx:

Preserved legacy behavior:

  • Single input that is a legacy input file, e.g.:
    • ./svOneDSolver inputFilename.in

Updated behavior:
Parse input args as:

  • -argName argValue -anotherArg anotherValue

Current options:

  • Run simulation with JSON input file:
    • -jsonInput inputFilename
  • Convert legacy input to JSON input:
    • -legacyToJson legacyInput.in jsonInput.json

Documentation

Documentation will be forthcoming in a follow-up submission once the input argument interfaces and JSON input files have been verified in the system tests.

Testing

Unit tests have been added for the JSON serialize/deserializer.

The existing system tests and unit tests cover the existing behavior and legacy input files.

No testing (including manual testing) has been done for the new input args. This will be managed in a subsequent submission which will add system tests that utilize the input args using equivalent JSON files for each of the existing system tests.

Next up

  • Python interfaces will be added that utilize the new input args.
  • Python interfaces will be added that perform the *.in -> *.json input file format conversion
  • As stated above, system tests will be added that utilize the new input args and the JSON files, so that it can be verified that they provide the input options in a way that is consistent with the requirements of the simulation clients.

Other follow-up

  • There are a few concerns for how we handle input options that are documented as comments in the code. We should potentially create issues for those and correct them.

Reviewers

@mrp089 @ktbolt

Code of Conduct & Contributing Guidelines

This change introduces a JSON parser and serializer for handling
input options. While the overall implementation is incomplete,
key functionality has been added.

Changes:
- Add nlohmann json to CMakeLists.txt
- Update to C++20
- Update CMake version as needed
- Add JsonParser for parsing a single options file
- Add JsonSerializer for serializing a single options file
- Refactor existing serialization to "LegacySerializer"
- Remove:
    cvDoubleVec jointXcoord
    cvDoubleVec jointYcoord
    cvDoubleVec jointZcoord
  from options (unused)
- Add "nodeAssociation" to joints list for explicit
  node associations in future updates

TODO:
- Add support for nested option files in JSON parser
- Add a converter for legacy format
- Update joint creation to use explicit node associations
- Update legacy parser to provide implicit node associations
  so they can be reused
- Verify changes against existing tests
- Add tests for parser, converter, etc.
…g legacy behavior. Add unit tests for json serialization.
@mrp089 mrp089 requested review from ktbolt and mrp089 February 18, 2025 02:56
Copy link
Contributor

@ktbolt ktbolt left a comment

Choose a reason for hiding this comment

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

Very good!

@mrp089
Copy link
Member

mrp089 commented Feb 18, 2025

@arsLibera, I have nothing to add. This looks very nice! Tagging #114

@mrp089 mrp089 merged commit d98fd52 into SimVascular:master Feb 18, 2025
3 checks passed
@arsLibera arsLibera mentioned this pull request Mar 16, 2025
1 task
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