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

Use configuration types as SSOT for defaults #833

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

stephenswat
Copy link
Member

As it stands, some of our default values for configuration parameters, e.g. those of the track finding, have defaults defined both in the configuration type as well as in the corresponding options type in the examples. This is confusing as @paradajzblond discovered today. In this commit, I remove the defaults defined in the examples code and treat the configuration type defaults as the single source of truth for default values.

@stephenswat stephenswat added improvement Improve an existing feature examples Changes to the examples labels Jan 31, 2025
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I like it. Just the m_ prefixes from my side... 🤔

@niermann999
Copy link
Contributor

This is another thing that might help the ITk track finding efficiency

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

👍

As it stands, some of our default values for configuration parameters,
e.g. those of the track finding, have defaults defined both in the
configuration type as well as in the corresponding options type in the
examples. This is confusing as @paradajzblond discovered today. In this
commit, I remove the defaults defined in the examples code and treat the
configuration type defaults as the single source of truth for default
values.
Copy link

sonarqubecloud bot commented Feb 3, 2025

@stephenswat stephenswat merged commit cdf632b into acts-project:main Feb 3, 2025
26 of 29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
examples Changes to the examples improvement Improve an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants