feat: full_chain_test.py config with command-line options #3811
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thinking longer term I believe these If we cannot validate the output of these chains they are just another unmonitored script. To try new algorithms and configurations one can modify a standard chain and if that workflow solidifies one can add a physmon workflow for this. |
|
We discussed this at some length at the Developers' meeting, with a variety of opinions.
I think these serve different use cases from physmon, but also this new
There is a real need for a command to perform interactive testing with simple command-line options. The evidence being how these scripts keep popping up (and not just written by me 😄). That may not be the way everyone works, but many of us do. I used my private version of this script to do all the tests on your recent CKF PRs - and I hope that was useful. Having private versions of that script is really inconvenient as people have to develop them independently, and keep them up to date.
Since this is a developer testing script, we'll have to provide it as-is with best-effort support. If it breaks when testing, then you fix it when you add/fix the feature you are testing. I am thinking of adding a big disclaimer to the top of the script and in the help text, eg.
|
I see that but I would argue that we should also delete all other
We had the case in the past already that people depended on the Overall I believe we are trying to oversimplify something which is inherently not simple and we just don't have the right abstraction for this right now. One reconstruction chain fulfills one purpose and we modify this by switching stuff in and out with numerous CLI args which cannot be tested and is ultimately bound to break. IMO modifying a chain is easiest if it is not overloaded with 100 CLI flags so you can just drop things our and copy something in without wiring another flag that potentially conflicts with 10 others. So I would argue the proposed script is not a solution to give a template for later to be modified chains. As for the interactive testing I can see that this is useful as you can simply play around with the reconstruction chain without knowing all the inner workings of ACTS and its Examples. But I would see this strictly for educational purposes or quick tests/checks for people with experience. |
|
If I understand you correctly, I think you might be confusing the purpose of this script, and what I understood was the original purpose of the existing scripts that I hope we can eventually restore.
It's 32 flags at the moment, and most of them are generic flags for changing the simulated sample (eg. For most use cases, it would not be necessary to wire in another CLI flag to perform the test. But it is useful to test with different single particle momenta, or with ttbar PU200, to run with different numbers of events, to try with ODD or ITk, to send the output to different directories so as not to overwrite previous tests. Without CLI flags, doing more than one interactive test becomes difficult to manage. Sometimes it might be useful to have a CLI flag to turn on and off a feature so one can run with and without the new feature. But that would be a temporary hack for that set of tests. I wondered whether it would be sometimes be useful to commit the new CLI flag to GitHub, but your comment convinces me that this would be a bad idea in the long term. So let's not do that. |
I see that but what I cannot see is how the situation is improved by keeping IMO to move forward with this the PR should also include the proposed changes to the other chains.
To me choosing a seeding algorithm and toggling the CKF is turning on and off particular details of the reconstruction. At the same time there are 3 ambi solvers to choose from. I don't think it is trivial to guarantee that all the combinations work (for example all seeders with/without CKF with ambi solver X). This is where I think the script does not offer any benefit other than educational purposes, which is a valid one. It is way too complicated to wire in another flag because it can easily break the script with specific options. |
Those are useful as easy-to-read examples for how to write your own script.
OK, I can make the simpler versions of these scripts and include it with this PR.
Specifying Your point may perhaps apply better to the As I said before, it isn't necessary to test all the options (or any). This is a developer tool. If something doesn't work for a developer, they can fix it. As I said in my proposed disclaimer, this should never be run in the CI, let alone production.
No, that is the purpose of the (simplified)
As I said, wiring in another flag isn't the primary use case. If it's too complicated for someone, then they won't do it. |
…ed by command-line options
|
I really don't understand how physmon can fail here... |
Simplifying |
|
@timadye that's merged now, so we should be good to go. |
|
full_chain_test.pysupports all options fromckf_tracks.py(generic detector),full_chain_odd.py, andfull_chain_itk.py, as well as some new additions to help with testing.The idea is to complement those scripts which provide examples, where
full_chain_test.pysimplifies running interactive/batch tests with different options specified on the command-line. With all the options, it is not intended to be an easy-to-read example. The existing example scripts could eventually be simplified, since they no longer need to support those test options they do have.The hope is that, in future,
full_chain_test.pycan be used to test new features and see the result in (at least) ODD and ITk environments.The idea for this initial version is:
full_chain_test.pyexactly matchesfull_chain_odd.pyfull_chain_test.py -A -M1 -N2exactly matchesfull_chain_itk.py-Aselects the ATLAS ITk configuration-M1 -N2(or--gen-nvertices 1 --gen-nparticles 2) changes to 2 particles per event fromfull_chain_odd.py's default of 800 (-M 200 -N 4)full_chain_test.py -Gis similarckf_tracks.py-Gselects the generic detector configurationckf_tracks.py, and doesn't try to reproduce seeding config.Later, if this becomes a useful development test, we can harmonise the detail setup between ODD and ITk.
Also, do we need to support options from
full_chain_itk_Gbts.pyandfull_chain_odd_LRT.py?