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

correct default settings within geometry_config.py #571

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anupama-reghunath
Copy link
Contributor

@anupama-reghunath anupama-reghunath commented Nov 28, 2024

Since within run_simScript.py we always pass the default value, the default settings within geometry_config.py were never used.

@anupama-reghunath anupama-reghunath requested a review from a team as a code owner November 28, 2024 16:03
@olantwin
Copy link
Contributor

These defaults are for backwards compatibility, to allow e.g. reconstructing old files before helium and removal of the SND were options, so I don't think these need updating.
Instead, we might consider throwing an error when trying to reconstruct these old files (and maybe give information this commit it was generated with, so that it can be reconstructed with the correct version).

@anupama-reghunath
Copy link
Contributor Author

atleast within the context of DecayVolume, the current version is not backwards compatible. 'vacuums' as default uses the current geometry for vacuum as opposed any of the older geometries with 'vacuums' as decayVolumeMedium.

@olantwin
Copy link
Contributor

Then at least for the DecayVolume we should probably just raise an error (with a helpful error message), instead of constructing an incompatible geometry.

@olantwin
Copy link
Contributor

@anupama-reghunath Any updates on this PR?

Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

Since we can still construct the SND, its default value for old geometries (which had an SND) should be True. @antonioiuliano2 what do you think?

Looks good to me otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants