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

refactor: RootMaterialDecorator as default for the ODD #3415

Merged

Conversation

benjaminhuth
Copy link
Member

getOpenDataDetector() gives by default the JsonMaterialDecorator, which does not give good results (at least in my use cases), and for most usecases, we anyways give directly the Root based one.

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Jul 19, 2024
@benjaminhuth
Copy link
Member Author

Tagging @andiwand @Corentin-Allaire

@benjaminhuth benjaminhuth added this to the next milestone Jul 19, 2024
andiwand
andiwand previously approved these changes Jul 19, 2024
@andiwand
Copy link
Contributor

Makes sense to me - thanks for making this sound @benjaminhuth

asalzburger
asalzburger previously approved these changes Jul 19, 2024
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Happy for the moment, though as discussed we should ship digi config + material config in the future with ACTS.

@asalzburger
Copy link
Contributor

Tagging @andiwand @Corentin-Allaire

That might change output slightly, but ok.

Anyhow - we again and again run into the situation that the ODD ships with files generated by ACTS which are strictly only valid for the ACTS version they are produced. Probably we should remove those and change to a setup where we guarantee that the standard material maps and digitisation files can be generated using ACTS for the ODD version it ships with?

@andiwand
Copy link
Contributor

Yes I think what should be done is to have the files in ODD and in ACTS. The parsing of these files are done by ACTS and this broke a couple of times in the past. This way the ACTS repo can have config files that work with the targeting ODD version and the current parser. The same way ODD repo can have a config file that works with the references ACTS version and the current ODD version.

@Corentin-Allaire
Copy link
Contributor

If we go that route (which I think is a great idea), maybe it would be worth having a CI run when we create a new tag to generate new material map for that tag/version ? That way we can at least ensure that in ACTS if you use a tag you are guaranty to have a working map ?
PS: technically we have (at least for the json one) in the material map a variable called format-version that was pu in place a long time ago with the idea that we would update it when we change the way the files were written. But unfortunatelly we never did. This might still be a useful option in the futur to solve the current issue

@Corentin-Allaire
Copy link
Contributor

Corentin-Allaire commented Jul 19, 2024

And just for your information, here the reason it doesn't work is because that json file is not a material map... It is a config file that should be use to tell the material mapping where you want to map your material. If this is read instead of a proper map it won't load any material.

So thanks for catching this ! And we should definitly change this !

@benjaminhuth
Copy link
Member Author

Okay very good to know, thx @Corentin-Allaire ! Just to be sure: the JsonMaterialDecorator is useful, right? So in principle there exist material descriptions in JSON?

@andiwand
Copy link
Contributor

It's interesting that JsonMaterialDecorator does not error 😄

@Corentin-Allaire
Copy link
Contributor

Corentin-Allaire commented Jul 19, 2024

@benjaminhuth, there is none in the odd directory as it take too much space (but it should be fully equivalent to the root one). But by default the material is written to json when the mapping is performed. So yes it is useful !

@andiwand the reason is that the way I implemented this is that when you configure the material mapping you actually decorate you detector with a placeholder material that is then pickup by the mapping. The mapping then see this material (which has the same binning as the one we want for our map) and understand it should replace it with the real material it computed. Long story short, in a way the config is a map but without physical material. (We could add a check to the material interactor to error if that typr is encountered)

@andiwand
Copy link
Contributor

I see makes sense @Corentin-Allaire. I wonder if we could in this case either have different I/O formats for input and output or instruct the reader to expect (valid) material properties. Do we use the same reader for mapping configuration and map reading?

@Corentin-Allaire
Copy link
Contributor

I see makes sense @Corentin-Allaire. I wonder if we could in this case either have different I/O formats for input and output or instruct the reader to expect (valid) material properties. Do we use the same reader for mapping configuration and map reading?

It is exactly the same format since they are both material map. But there will be a difference at the level of material class, in the config the surfaces will be associated with ProtoSurfaceMaterial (in json : "type": "proto") while in a proper map it will be BinnedSurfaceMaterial. One could just check the detector for proto material at the construction to let the user know

@benjaminhuth benjaminhuth dismissed stale reviews from asalzburger and andiwand via b663b1d July 19, 2024 11:48
@kodiakhq kodiakhq bot merged commit b46d425 into acts-project:main Jul 19, 2024
43 checks passed
Copy link

sonarcloud bot commented Jul 19, 2024

@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results [b46d425]

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jul 19, 2024
@paulgessinger paulgessinger modified the milestones: next, v36.1.0 Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Examples Affects the Examples module Needs Discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants