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: Update Json converters and enhance Detray version writing #2608

Merged

Conversation

asalzburger
Copy link
Contributor

@asalzburger asalzburger commented Nov 1, 2023

This PR updates the Json writers for the new Detector geometry, and particular for the detray export.

The final output needs still a bit fine-tuning and the material files are still empty.

It adds also support for the IndexedRootVolumes, a grid to find the initial volume to start from, it uses the same infrastructure as the IndexedSurfacesUpdators hence this code was centrally put into a helper function.

@asalzburger asalzburger added this to the next milestone Nov 1, 2023
@github-actions github-actions bot added Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins labels Nov 1, 2023
@asalzburger asalzburger removed the request for review from dimitra97 November 1, 2023 12:48
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ecdbde7) 49.73% compared to head (8b37198) 49.73%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2608   +/-   ##
=======================================
  Coverage   49.73%   49.73%           
=======================================
  Files         474      474           
  Lines       26874    26874           
  Branches    12365    12365           
=======================================
  Hits        13366    13366           
  Misses       4704     4704           
  Partials     8804     8804           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@asalzburger
Copy link
Contributor Author

Needs unit test fixing.

@paulgessinger
Copy link
Member

Could you attach a sample json file to the PR? Just wondering what this ends up looking like.
If I have some time, I might look into writing a schema to validate the format.

@asalzburger
Copy link
Contributor Author

Could you attach a sample json file to the PR? Just wondering what this ends up looking like. If I have some time, I might look into writing a schema to validate the format.

Yes, I will, there's still some refinement to be done in the interplay with detray, so we will do this once the chain is closed.

@niermann999
Copy link
Contributor

Could you attach a sample json file to the PR? Just wondering what this ends up looking like. If I have some time, I might look into writing a schema to validate the format.

Some Json schemas for the detray files exist in https://github.com/acts-project/detray/tree/main/tests/validation/python/validation/json_schema

@paulgessinger
Copy link
Member

@niermann999 now that is excellent! Is this the standard json schema format? It might make sense to put them into separate JSON files so they are usable with standard command line tools.

@niermann999
Copy link
Contributor

@niermann999 now that is excellent! Is this the standard json schema format? It might make sense to put them into separate JSON files so they are usable with standard command line tools.

As far as I know it is (I used https://json-schema.org to write the thing). Up to this point there is a small python executable that runs the validation on a given input json file using the jsonschema module

@github-actions github-actions bot added the Component - Core Affects the Core module label Nov 14, 2023
niermann999
niermann999 previously approved these changes Nov 14, 2023
@acts-policybot acts-policybot bot dismissed niermann999’s stale review November 14, 2023 20:30

Invalidated by push of 445f355

@asalzburger
Copy link
Contributor Author

Sorry @niermann999 - needs re-approving, I fixed clang-tidy and doc build.

niermann999
niermann999 previously approved these changes Nov 15, 2023
@asalzburger asalzburger force-pushed the feat-update-json-detray-converters branch from a538d42 to 20c62cc Compare November 16, 2023 06:11
@acts-policybot acts-policybot bot dismissed niermann999’s stale review November 16, 2023 06:11

Invalidated by push of 20c62cc

@asalzburger
Copy link
Contributor Author

This ran into CI/bridge hard limit again, it seems, the bridge jobs didn't even start.

@asalzburger
Copy link
Contributor Author

Since all other tests ran through, and it has been two days waiting for merging - I do this manually now.

@asalzburger asalzburger merged commit 1c81427 into acts-project:main Nov 16, 2023
35 of 36 checks passed
@asalzburger asalzburger deleted the feat-update-json-detray-converters branch November 16, 2023 21:18
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Nov 16, 2023
@paulgessinger paulgessinger modified the milestones: next, v31.0.0 Nov 17, 2023
@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Nov 20, 2023
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.

4 participants