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: Introduce Json Converter for Surfaces #662

Merged

Conversation

asalzburger
Copy link
Contributor

This PR sits on top of #660 and introduces Json converters for Surfaces.

It comes with unit tests that show the round test for all surface types to work.

@asalzburger asalzburger added this to the next milestone Jan 21, 2021
@asalzburger asalzburger added Component - Plugins Affects one or more Plugins Feature Development to integrate a new feature labels Jan 21, 2021
@asalzburger asalzburger self-assigned this Jan 21, 2021
@asalzburger asalzburger added the 🚧 WIP Work-in-progress label Jan 21, 2021
@asalzburger
Copy link
Contributor Author

asalzburger commented Jan 21, 2021

@Corentin-Allaire this one is waiting for the Material converters which can be plugged in, actually I think we will be able to transport the full geometry in Json at some point (and read it back in).

@Corentin-Allaire
Copy link
Contributor

@Corentin-Allaire this one is waiting for the Material converters which can be plugged in, actually I think we will be able to transport the full geometry in Json at some point (and read it back in).

I have the material converter in the PR that am finishing, just need to update the CI and the initialisation script for the mat map. Would you have 5min after tomorrow's morning meeting to discuss what we want to do with the json conversion ?

@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #662 (7bc111a) into master (3ddff2e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #662   +/-   ##
=======================================
  Coverage   49.06%   49.06%           
=======================================
  Files         329      329           
  Lines       16438    16438           
  Branches     7657     7657           
=======================================
  Hits         8066     8066           
  Misses       2998     2998           
  Partials     5374     5374           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ddff2e...c60a4eb. Read the comment docs.

Copy link
Contributor

@Corentin-Allaire Corentin-Allaire left a comment

Choose a reason for hiding this comment

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

Looks good to me. I think this should go in before #667, I might be able to modify it to use the surfaces.

/// @param j the read-in json object
///
/// @return a shared_ptr to a surface object for type polymorphism
std::shared_ptr<Surface> surfaceFromJson(const nlohmann::json&);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could also instead of surfaceFromJson use from_json(const nlohmann::json& j, std::shared_ptr<Surface>& t) not sure it would be better but that is an option.

@asalzburger
Copy link
Contributor Author

I actually wanted to make this clear that it's sort of a different way, but you're right. Will change it, I think.

@asalzburger
Copy link
Contributor Author

Thinking about it - not really, one would have to provide a nullptr which is then unnulled - not a big fan.

@asalzburger asalzburger merged commit 4bd3a88 into acts-project:master Jan 22, 2021
@asalzburger asalzburger deleted the feat-surface-json-converter branch January 22, 2021 17:59
@paulgessinger paulgessinger modified the milestones: next, v5.0.0 Jan 29, 2021
asalzburger pushed a commit that referenced this pull request Feb 4, 2021
This PR modify the JsonGeometryConverter that is used in the material mapping process to use the geometry hierarchy this give us a more uniform Json output compare to other Acts json files.

The python script used to configure which surface with which binning to use for the material mapping have been modify to accommodate the file structure.

I modify the key used for the ID in GeometryHierarchyMapJsonConverter.hpp, with the previous key the element would be order alphabetically in the file (which made them extremely hard to read). In the same way some key used in the writing of the material mapping use Capital letter, this is not a mistake it is to for the element to be ordered in a more readable way in the files.

Added 2 files with to_json and from_json, the first one is used to convert material, material surface and material volume. The second convert pointer on surface and tracking volume to initialise the material map json file used to configure the mapping surface/volume and binning. This shouldn't conflict with #662 but we should keep an eye on it.
asalzburger pushed a commit to asalzburger/acts that referenced this pull request Feb 8, 2021
This PR modify the JsonGeometryConverter that is used in the material mapping process to use the geometry hierarchy this give us a more uniform Json output compare to other Acts json files.

The python script used to configure which surface with which binning to use for the material mapping have been modify to accommodate the file structure.

I modify the key used for the ID in GeometryHierarchyMapJsonConverter.hpp, with the previous key the element would be order alphabetically in the file (which made them extremely hard to read). In the same way some key used in the writing of the material mapping use Capital letter, this is not a mistake it is to for the element to be ordered in a more readable way in the files.

Added 2 files with to_json and from_json, the first one is used to convert material, material surface and material volume. The second convert pointer on surface and tracking volume to initialise the material map json file used to configure the mapping surface/volume and binning. This shouldn't conflict with acts-project#662 but we should keep an eye on it.
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Feb 9, 2021
This PR modify the JsonGeometryConverter that is used in the material mapping process to use the geometry hierarchy this give us a more uniform Json output compare to other Acts json files.

The python script used to configure which surface with which binning to use for the material mapping have been modify to accommodate the file structure.

I modify the key used for the ID in GeometryHierarchyMapJsonConverter.hpp, with the previous key the element would be order alphabetically in the file (which made them extremely hard to read). In the same way some key used in the writing of the material mapping use Capital letter, this is not a mistake it is to for the element to be ordered in a more readable way in the files.

Added 2 files with to_json and from_json, the first one is used to convert material, material surface and material volume. The second convert pointer on surface and tracking volume to initialise the material map json file used to configure the mapping surface/volume and binning. This shouldn't conflict with acts-project#662 but we should keep an eye on it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Plugins Affects one or more Plugins Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants