-
Notifications
You must be signed in to change notification settings - Fork 39
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
allow disabling sky signal from the command line #313
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.
Also, should adding the conviqr or the pysm args trigger adding this as well?
): | ||
""" Use PySM to simulate smoothed sky signal. | ||
|
||
""" | ||
if not args.pysm_model: | ||
if not args.pysm_model or not args.simulate_sky: | ||
return None |
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 You are modifying this, what about replace return None with return only?
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.
Explicit is better than implicit (https://www.python.org/dev/peps/pep-0020) especially when the method usually has a return value. Now that you brought this up, I'll go add None
to https://github.com/hpc4cmb/toast/blob/sky_signal_args/src/toast/pipeline_tools/sky_signal.py#L252-L255
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.
Conviqt and PySM args already add these: https://github.com/hpc4cmb/toast/blob/sky_signal_args/src/toast/pipeline_tools/sky_signal.py#L136 and https://github.com/hpc4cmb/toast/blob/sky_signal_args/src/toast/pipeline_tools/sky_signal.py#L245 respectively.
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.
I agree with the first answer to https://stackoverflow.com/questions/15300550/return-return-none-and-no-return-at-all.
Here we are breaking out of a func so I would use return.
Most pipeline_tools packages allow disabling the module from the command line. This PR adds that functionality to PySM, conviqt and map sampling: regardless of what arguments are specified before, if the user extends the command line with
--no-sky
or--no-simulate-sky
, then the sky simulation is disabled.