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

feat: adding json writing, reading infrastructure #2283

Merged
merged 41 commits into from
Jul 17, 2023

Conversation

asalzburger
Copy link
Contributor

This PR adds the reading/writing infrastructure of the new Experimental detector feature to and from json.

At the same time it streamlines how we use the json::nlohmann module:

  • As we need polymorphism, the intrinsic to_json and from_json nomenclature that would allow auto-translation is practically unusable for the Detector. Hence it is replaced by a consistent ObjectJsonConverter::toJson and ObjectJsonConverter::fromJson naming scheme.
  • It changes enum types from hand-written string writing to the nlohmann::enum handling macro consistently
  • It introduces nested Options struct for future refinement of json writing
  • It adds a dedicated detray writing mode for conversion into detray formal

@asalzburger asalzburger added this to the next milestone Jul 6, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins labels Jul 6, 2023
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

📊 Physics performance monitoring for a68ef55

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@paulgessinger
Copy link
Member

How does polymorphism prevent us from using to_json / from_json? Don't you have to downcast anyway, and this can't be done inside these functions? The benefit of this ADL approach is that nlohmann::json automatically handles the recursion for you, which is nice.

@codecov
Copy link

codecov bot commented Jul 7, 2023

Codecov Report

Merging #2283 (a68ef55) into main (8833e70) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2283      +/-   ##
==========================================
- Coverage   49.33%   49.30%   -0.03%     
==========================================
  Files         447      447              
  Lines       25149    25162      +13     
  Branches    11571    11577       +6     
==========================================
  Hits        12407    12407              
- Misses       4542     4555      +13     
  Partials     8200     8200              
Impacted Files Coverage Δ
...nclude/Acts/Navigation/NavigationStateUpdators.hpp 66.66% <ø> (ø)
Core/src/Detector/detail/PortalHelper.cpp 35.48% <0.00%> (-25.63%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@asalzburger
Copy link
Contributor Author

How does polymorphism prevent us from using to_json / from_json? Don't you have to downcast anyway, and this can't be done inside these functions? The benefit of this ADL approach is that nlohmann::json automatically handles the recursion for you, which is nice.

I encountered two problems:

  • first neither to_json nor from_json can handle extra arguments (which we need, an easy example is the GeometryContext, but way more important are relationship/associations, e.g. to which volume a portal points, etc.)
  • if specific types (e.g. a specific BoundType has to be constructed, I anyway need to interpret the json first, then call a dedicated method, which sometimes I could solve with providing the template type, see e.g. the SurfaceBounds re-creation)
  • then, we handle polymorphic objects in the return, which can not be created by BaseClass(nlohmann::json("data") and hence an explicit call is needed.

If you look through the JsonPlugin directory, there's only very few cases the the implicit to_json / from_json is actually still in place, in fact only for rather primitive objects. The rest is already customised, but was not done consistently. This PR tries to put at areas all relevant converters for the new Detector schema into one consistent way of handling it.

@andiwand
Copy link
Contributor

andiwand commented Jul 7, 2023

I think the idea of to_json and from_json is that you do not require any config to parse something. It has to be serialized in a way that everything is self-contained. For the geometry context this is a problem. In case of writing it might still be feasible with to_json(std::pair<Object, Context> cpp) but the for reading it would already be more funky

std::pair<Object, Context> cpp;
cpp.second = ...some context...;
from_json(json, cpp);

In case of polymorphism you either need to know what type you are parsing or you need a base class parser that can deduce the actual type. But maybe I did not understand the application in this case.

@paulgessinger
Copy link
Member

paulgessinger commented Jul 7, 2023

  • The pattern we've used in the past for extra arguments like the geo context is to define to_json on a tuple like to_json(std::tuple<GeometryContext, Object>).
  • Right, you'd call j.get<MyType>(), to have the library invoke from_json for you. Is this not sufficient?
  • I'm indeed not sure if base classes can be constructed with from_json, but is this really needed? You have to switch/case anyway to find the concrete type from one of the JSON values, and this can be done in the parent object from_json, which then just calls .get<ConcreteType>(), and up-casts when the constructed value is assigned.

I don't know. Ultimately, to_json / from_json is a pretty robust design and is directly integrated into the nlohmann::json library. I personally think if at all possible we should stick to that, but it's up to you of course.

@asalzburger
Copy link
Contributor Author

asalzburger commented Jul 7, 2023

  • The pattern we've used in the past for extra arguments like the geo context is to define to_json on a tuple like to_json(std::tuple<GeometryContext, Object>).
  • Right, you'd call j.get<MyType>(), to have the library invoke from_json for you. Is this not sufficient?
  • I'm indeed not sure if base classes can be constructed with from_json, but is this really needed? You have to switch/case anyway to find the concrete type from one of the JSON values, and this can be done in the parent object from_json, which then just calls .get<ConcreteType>(), and up-casts when the constructed value is assigned.

I don't know. Ultimately, to_json / from_json is a pretty robust design and is directly integrated into the nlohmann::json library. I personally think if at all possible we should stick to that, but it's up to you of course.

Certainly worth the discussion!

If we manage the use the native 'to_json' / 'from_json' design - I am certainly up for it.

Let me try to summarise what is needed:

  • I have output objects, which sometimes have to be constructed as shared_ptr or unique_ptr in order to make them useful downstream
  • I have addition information (associations) that these objects need to be written out and read in correctly
  • I have options parameter that steer the output (partly to switch things on/off, partly to manipulate)

So, my design for this was so far:

Writing:
ObjectJsonConverter::toJson(const Object&, [... association objects ...], const Options& options)

Reading:
auto object = ObjectJsonConverter::fromJson(const nlohmann::json& json, [ ... association objects ...])

For the reading, e.g. association objects could be the already created volumes, such that the links to those can be restored. How this is done in the detector reading, is that e.g. all volumes are constructed first, then all portals will be constructed and the list of volumes is provided at portal construction.

If I understand that correctly I would do a tuple object and define read/write for that ?

@asalzburger
Copy link
Contributor Author

I think the idea of to_json and from_json is that you do not require any config to parse something. It has to be serialized in a way that everything is self-contained.

The bigger problem is actually that we can not serialise everything on an object level, the GeometryContext is the smallest problem here.

Think of our Detector/TrackingGeometry concept: volumes can share portal surfaces which then point to volumes again. My custom toJson/fromJson methods allows me to do that in a clever way, i.e. I can resuse and optimise information, e.g. I can write a single portal container to the output json and then, when the actual portals of the volumes are written, I only write the index into these containers to the volumes (this requires the volume to be written in the context of a detector setup).

Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

to be continued

Core/src/Detector/detail/PortalHelper.cpp Show resolved Hide resolved
Plugins/Json/src/DetectorJsonConverter.cpp Outdated Show resolved Hide resolved
Plugins/Json/src/DetectorJsonConverter.cpp Outdated Show resolved Hide resolved
Plugins/Json/src/DetectorVolumeJsonConverter.cpp Outdated Show resolved Hide resolved
Plugins/Json/src/DetectorVolumeJsonConverter.cpp Outdated Show resolved Hide resolved
asalzburger and others added 7 commits July 7, 2023 17:17
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
…er.hpp

Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
asalzburger and others added 4 commits July 8, 2023 13:46
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

I am wondering if the detray implementation should go into its own file, at least for the tests, so that detray conversion bugs don't fail the general tests?

Plugins/Json/src/GridJsonConverter.cpp Outdated Show resolved Hide resolved
Plugins/Json/src/GridJsonConverter.cpp Outdated Show resolved Hide resolved
Plugins/Json/src/IndexedSurfacesJsonConverter.cpp Outdated Show resolved Hide resolved
Plugins/Json/src/IndexedSurfacesJsonConverter.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Plugins/Json/PortalJsonConverterTests.cpp Outdated Show resolved Hide resolved
@asalzburger
Copy link
Contributor Author

@niermann999 - all comments addressed I suppose.

Copy link
Contributor

@niermann999 niermann999 left a comment

Choose a reason for hiding this comment

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

Otherwise, I think this can go forward

@asalzburger asalzburger merged commit bb68534 into acts-project:main Jul 17, 2023
53 checks passed
@paulgessinger paulgessinger modified the milestones: next, v27.2.0 Jul 24, 2023
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 24, 2023
This PR adds the reading/writing infrastructure of the new
`Experimental` detector feature to and from json.

At the same time it streamlines how we use the `json::nlohmann` module:

- As we need polymorphism, the intrinsic `to_json` and `from_json`
nomenclature that would allow auto-translation is practically unusable
for the `Detector`. Hence it is replaced by a consistent
`ObjectJsonConverter::toJson` and `ObjectJsonConverter::fromJson` naming
scheme.
- It changes enum types from hand-written string writing to the
`nlohmann::enum` handling macro consistently
- It introduces nested `Options` struct for future refinement of json
writing
- It adds a dedicated `detray` writing mode for conversion into detray
formal

---------

Co-authored-by: Joana Niermann <53186085+niermann999@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants