fix: Fix BinningValue conversions
#2333
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the ATLAS debug build, we're getting failures in the ActsEventCnv test: ``` ActsTrackingGeometrySvc FATAL in sysInitialize(): standard std::exception is caught ActsTrackingGeometrySvc ERROR [json.exception.type_error.302] type must be number, but is string ServiceManager ERROR Unable to initialize service "ActsTrackingGeometrySvc" ``` This is because some compilation units in Plugins/Json try to convert a BinningValue without including the header UtilitiesJsonConverter.hpp which has the NLOHMANN_JSON_SERIALIZE_ENUM declaration for BinningValue. Thus, the conversion can work differently depending on the compilation unit, and the behavior can also depend on what gets inlined, so can differ between the optimized and debug build. This change adds the missing #includes and should fix this failure.
andiwand
changed the title
BinningValue conversions.BinningValue conversions
Codecov Report
@@ Coverage Diff @@
## main #2333 +/- ##
=======================================
Coverage 49.67% 49.67%
=======================================
Files 453 453
Lines 25524 25524
Branches 11701 11701
=======================================
Hits 12678 12678
Misses 4571 4571
Partials 8275 8275 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
📊 Physics performance monitoring for 5b4bc27Summary VertexingVertexing vs. muIVF seededIVF truth_smearedIVF truth_estimatedIVF orthogonalAMVF seededAMVF truth_smearedAMVF truth_estimatedAMVF orthogonalSeedingSeeding seededSeeding truth_estimatedSeeding orthogonalCKFCKF seededCKF truth_smearedCKF truth_estimatedCKF orthogonalAmbiguity resolutionseededTruth tracking (Kalman Filter)Truth trackingTruth tracking (GSF)Truth tracking |
paulgessinger
approved these changes
Jul 29, 2023
acts-project-service
added
the
Fails Athena tests
|
Pretty sure the output changes are not related. |
paulgessinger
removed
the
Fails Athena tests
This was referenced Apr 25, 2024
kodiakhq bot
pushed a commit
that referenced
this pull request
Apr 26, 2024
Since we had the same enum conversion problem a second time I propose to disable the default enum serialization via `JSON_DISABLE_ENUM_SERIALIZATION`. This results in a compile error instead of runtime errors when we actually try to read strings as integers. I set the flag `PRIVATE` in cmake so it does not sneak into user codebases but the user might face the same problems if the includes are not done correctly. Not having the enum and the `NLOHMANN_JSON_SERIALIZE_ENUM` in the same place results in these kind of issues. I wonder if we should rather bring them together and use preprocessor flags. That would also guarantee that the right conversion function will be visible all the time. References: - https://json.nlohmann.me/api/macros/json_disable_enum_serialization/ - https://json.nlohmann.me/api/macros/nlohmann_json_serialize_enum/ Related PRs and issues: - #3142 - #2333 - #2331
EleniXoch
pushed a commit
to EleniXoch/acts
that referenced
this pull request
May 6, 2024
) Since we had the same enum conversion problem a second time I propose to disable the default enum serialization via `JSON_DISABLE_ENUM_SERIALIZATION`. This results in a compile error instead of runtime errors when we actually try to read strings as integers. I set the flag `PRIVATE` in cmake so it does not sneak into user codebases but the user might face the same problems if the includes are not done correctly. Not having the enum and the `NLOHMANN_JSON_SERIALIZE_ENUM` in the same place results in these kind of issues. I wonder if we should rather bring them together and use preprocessor flags. That would also guarantee that the right conversion function will be visible all the time. References: - https://json.nlohmann.me/api/macros/json_disable_enum_serialization/ - https://json.nlohmann.me/api/macros/nlohmann_json_serialize_enum/ Related PRs and issues: - acts-project#3142 - acts-project#2333 - acts-project#2331
asalzburger
pushed a commit
to asalzburger/acts
that referenced
this pull request
May 21, 2024
) Since we had the same enum conversion problem a second time I propose to disable the default enum serialization via `JSON_DISABLE_ENUM_SERIALIZATION`. This results in a compile error instead of runtime errors when we actually try to read strings as integers. I set the flag `PRIVATE` in cmake so it does not sneak into user codebases but the user might face the same problems if the includes are not done correctly. Not having the enum and the `NLOHMANN_JSON_SERIALIZE_ENUM` in the same place results in these kind of issues. I wonder if we should rather bring them together and use preprocessor flags. That would also guarantee that the right conversion function will be visible all the time. References: - https://json.nlohmann.me/api/macros/json_disable_enum_serialization/ - https://json.nlohmann.me/api/macros/nlohmann_json_serialize_enum/ Related PRs and issues: - acts-project#3142 - acts-project#2333 - acts-project#2331
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the ATLAS debug build, we're getting failures in the ActsEventCnv test:
This is because some compilation units in Plugins/Json try to convert a BinningValue without including the header UtilitiesJsonConverter.hpp which has the NLOHMANN_JSON_SERIALIZE_ENUM declaration for BinningValue. Thus, the conversion can work differently depending on the compilation unit, and the behavior can also depend on what gets inlined, so can differ between the optimized and debug build.
fixes #2331