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

Update nlohmann/json library (fixes #284) #424

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

tquatmann
Copy link
Member

I re-applied the changes made to the old json version concerning RationalNumber support.

My changes to the main nlohmann/json release can be seen here

I briefly checked for obvious performance regressions, which does not seem to be the case.
Here some data, where I used storm-conv to parse and then export a ~38 MB JANI file from QVBS:

current main:

Command line arguments: --jani oscillators.8-10-0.1-1.jani --tojani test1 -jprop --compactjson
Current working directory: /Users/tim/git/qcomp/benchmarks/dtmc/oscillators

Parsing JANI input ...  done. (2.175s seconds).
Performing transformations on JANI model ...  done. (0.201s seconds).
Exporting JANI model ... Stored to file 'test1' done. (1.268s seconds).

This branch:

Command line arguments: --jani oscillators.8-10-0.1-1.jani --tojani test1 -jprop --compactjson
Current working directory: /Users/tim/git/qcomp/benchmarks/dtmc/oscillators

Parsing JANI input ...  done. (2.199s seconds).
Performing transformations on JANI model ...  done. (0.200s seconds).
Exporting JANI model ... Stored to file 'test1' done. (1.081s seconds).
?master ~/git/qcomp/benchmarks/dtmc/oscillators> 

@tquatmann
Copy link
Member Author

tquatmann commented Nov 10, 2023

I still need to add some documentation here before merging this.

@sjunges
Copy link
Contributor

sjunges commented Nov 11, 2023

You are a true hero :-)

@sjunges
Copy link
Contributor

sjunges commented Nov 11, 2023

Is there any reason why we did not specialize std::is_scalar_v in the changes within modernjson? That would have saved quite some lines in the diff?

Likewise, should we consider adding support for std::isfinite and some of the other number traits?

@sjunges
Copy link
Contributor

sjunges commented Nov 11, 2023

Your changes in this PR LGTM

@tquatmann
Copy link
Member Author

specialising is_scalar (and similar traits) would be great, but (quoting cppreference ):

The behavior of a program that adds specializations for std::is_scalar or std::is_scalar_v is undefined.

@tquatmann
Copy link
Member Author

I made the json export a bit more robust (in case a huge storm::RationalNumber like 1E5000 might be converted to something like nan and/or inf.

I also added the promised documentation. From my side, this is ready to be merged :)

@sjunges
Copy link
Contributor

sjunges commented Nov 15, 2023

LGTM!

@tquatmann tquatmann merged commit cbb9571 into moves-rwth:master Nov 17, 2023
14 checks passed
@marisgg
Copy link

marisgg commented Nov 21, 2023

Hey, I thought I'd share the following; hopefully, this will help with debugging.

This Stormpy master fork fails (https://github.com/randriu/stormpy/actions/runs/6945132931/job/18893799741) on the latest version of Storm CI, possibly due to this PR. The error is inside CMakeFiles/utility.dir/src/utility/json.cpp.

I'm assuming this very recently started Stormpy build (https://github.com/moves-rwth/stormpy/actions/runs/6945494052/job/18894977397) will fail because of this.

@sjunges
Copy link
Contributor

sjunges commented Nov 21, 2023

Looking at this, the problem is here: https://github.com/randriu/stormpy/blob/master/src/utility/json.cpp but I dont know where exactly. Maybe @tquatmann sees it?

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