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: Python TGeoDetector.create() can read config from JSON file #1095

Merged
merged 3 commits into from
Dec 3, 2021

Conversation

timadye
Copy link
Contributor

@timadye timadye commented Dec 3, 2021

  • TGeoDetector.create(jsonFile=tgeo.json) fills the config from the JSON file. Other settings can still be set from Python.
  • Fixes a small issue with the _detector_create adapter, which didn't recognise the jsonFile setter with no getter.

* `TGeoDetector.create(jsonFile=tgeo.json)` fills the config from the JSON file. Other settings can still be set from Python.
* Fixes a small issue with the `_detector_create` adapter, which didn't recognise the `jsonFile` setter with no getter.
@timadye timadye added this to the next milestone Dec 3, 2021
@paulgessinger
Copy link
Member

paulgessinger commented Dec 3, 2021

In principle, why not.

But if you're using the python bindings anyway, I guess I would suggest just using python to construct the TGeo configuration, which should be much more readable/maintainable than the JSON version in my opinon? (see here)

@timadye
Copy link
Contributor Author

timadye commented Dec 3, 2021

But if you're using the python bindings anyway, I guess I would suggest just using python to construct the TGeo configuration, which should be much more readable/maintainable than the JSON version in my opinon?

Agreed. I found this useful to load an existing ITk configuration in JSON format.

(see here)

That didn't work for me, so I added this option to compare. The JSON version works for me, so this helps as a fallback and/or to debug the Python one.

timadye pushed a commit to timadye/acts that referenced this pull request Dec 3, 2021
* Implemented in `ckf_tracks.py`, which previously worked just for the Generic Detector. The changes should be compatible with existing tests using `ckf_tracks.py`.
* Config can be specified with command-line options.
* Use `ckf_tracks.py --atlas` to select options to load config from `acts-detector-examples` (get in touch if you don't have this).
* Requires acts-project#1095 to use with `--atlas` and without `--build-itk-geometry`.
@codecov
Copy link

codecov bot commented Dec 3, 2021

Codecov Report

Merging #1095 (3d6bb76) into main (fa7ca7f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1095   +/-   ##
=======================================
  Coverage   48.58%   48.58%           
=======================================
  Files         341      341           
  Lines       17515    17515           
  Branches     8263     8263           
=======================================
  Hits         8510     8510           
  Misses       3232     3232           
  Partials     5773     5773           

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 fa7ca7f...3d6bb76. Read the comment docs.

@kodiakhq kodiakhq bot merged commit f8ff231 into acts-project:main Dec 3, 2021
timadye pushed a commit to timadye/acts that referenced this pull request Dec 6, 2021
* Implemented in `ckf_tracks.py`, which previously worked just for the Generic Detector. The changes should be compatible with existing tests using `ckf_tracks.py`.
* Config can be specified with command-line options.
* Use `ckf_tracks.py --atlas` to select options to load config from `acts-detector-examples` (get in touch if you don't have this).
* Requires acts-project#1095 to use with `--atlas` and without `--build-itk-geometry`.
@paulgessinger paulgessinger modified the milestones: next, v15.1.0 Dec 10, 2021
timadye pushed a commit to timadye/acts that referenced this pull request Dec 16, 2021
* Implemented in `ckf_tracks.py`, which previously worked just for the Generic Detector. The changes should be compatible with existing tests using `ckf_tracks.py`.
* Config can be specified with command-line options.
* Use `ckf_tracks.py --atlas` to select options to load config from `acts-detector-examples` (get in touch if you don't have this).
* Requires acts-project#1095 to use with `--atlas` and without `--build-itk-geometry`.
timadye pushed a commit to timadye/acts that referenced this pull request Feb 3, 2022
* Implemented in `ckf_tracks.py`, which previously worked just for the Generic Detector. The changes should be compatible with existing tests using `ckf_tracks.py`.
* Config can be specified with command-line options.
* Use `ckf_tracks.py --atlas` to select options to load config from `acts-detector-examples` (get in touch if you don't have this).
* Requires acts-project#1095 to use with `--atlas` and without `--build-itk-geometry`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants