-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add CLI options to configuration type conversion #626
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 It's a neat idea...
But I would leave traccc::opts::clusterization
alone in this PR. 🤔 I think in its current form it just confuses things more than it helps.
We've talked about this with @beomki-yeo in passing in the past. That generally every algorithm should come with a configuration struct eventually. Instead of some of them having such structs, and some of them just receiving some primitive variables.
In such a setup, where there would be traccc::<algorithm>::config
types for everything, these auto-converter operators would result in a very nice code layout indeed. But for just handling a single unsigned short
, doing this change (in my opinion) just makes the code harder to read.
The changes to traccc::opts::track_finding
and traccc::opts::track_propagation
are very interesting on the other hand. 👍
I'm glad you like it. I agree that doing this for a single short is silly, but #595 will expand the number of options to the clustering algorithm. Using this configuration provider pattern for the clustering will make it trivially easy to implement the new config in #595, although you are correct that making the change later is not much more difficult. So if you prefer I can revert the change for clustering in this PR and move that to #595; otherwise we can leave it here. No real different either way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's update traccc::ops::clusterization
in this PR already then. I'm now just wondering about possibly shortening the example executable code even further. 🤔
I.e. is it necessary in any of the cases to create a separate detray::propagation::config
object? I'm not actually sure...
As I am finalizing acts-project#595, I am noticing that it is really quite a task to update the configuration of our algorithms. Not only do you need to update the configuration type and the ways your algorithm uses it, but you also need to update the way the CLI options are translated into those configuration files across _many_ executables. In this commit I am trying to make this process a little easier by specifying the `config_provider` mixin for command line options classes. This allows us to specify only once how options should be translated to configuration types, making the process easier and less prone to bugs.
2518e6e
to
711d554
Compare
Updated 6 instances where a config object was created and used only once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
As I am finalizing #595, I am noticing that it is really quite a task to update the configuration of our algorithms. Not only do you need to update the configuration type and the ways your algorithm uses it, but you also need to update the way the CLI options are translated into those configuration files across many executables.
In this commit I am trying to make this process a little easier by specifying the
config_provider
mixin for command line options classes. This allows us to specify only once how options should be translated to configuration types, making the process easier and less prone to bugs.