Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Coadd1d bad spec1d #1646
Coadd1d bad spec1d #1646
Changes from 22 commits
648bbcc
5e650ee
c8a27cb
79da2ee
480a075
7122f14
b6529f7
0d7519d
23da51c
3fb2fa2
96c21e6
5e4b6f4
ef469a1
71ca995
f96bc6b
fb0d77a
9fdc6f9
1ab4836
9f445b4
13e756d
00495cc
8a7ae8d
4a03e43
5351e1e
1aec242
6b55d6d
d0a7e90
6687db3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This masking based on a statistical analysis of the rms_sn needs to NOT be the default behavior. My suggestion is that you set sigrej_exp to default to None. And that you only carry out the code below if sigrej_exp is not 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.
Masking is not done if
sigrej_exp=None
. You can see below:There is still some stats on the S/N, which is printed in the terminal, when
sigrej_exp=None
. I think it's useful to have that information, but the masking is not applied.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 I agree printing the information is useful. I would some text to the warning if sigrej_exp is not None that indicates the exposure is being masked. Right now the user won't know it is masked from watching the terminal outputs.
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.
See my feedback on your previous version of this PR. You don't need to assign sn_smooth_pix since that is used for the weights, not for the rms_sn, for which the whole spectrum is used. So all of this sn_smooth_pix should be removed. You should instead write:
rms_sn, _ = coadd.sn_weights(fluxes, ivars, gpms, const_weights=True)
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.
Sorry I missed that. I removed
sn_smooth_pix
.Thanks!