-
Notifications
You must be signed in to change notification settings - Fork 18
adding the use of psd from preprocessing to use as mapmaking weights #1224
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
Conversation
… Also a repeated NmatWhite class is removed
… Also a repeated NmatWhite class is removed
…obs/sotodlib into chervias/weights_with_psd
Ok, this is ready for review now |
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.
Looks good to me.
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.
Just a boring formatting + docstring request...
sotodlib/mapmaking/demod_mapmaker.py
Outdated
@@ -64,7 +64,7 @@ def __init__(self, signals=[], noise_model=None, dtype=np.float32, verbose=False | |||
self.ncomp = len(comps) | |||
self.singlestream = singlestream | |||
|
|||
def add_obs(self, id, obs, noise_model=None, split_labels=None): | |||
def add_obs(self, id, obs, noise_model=None, split_labels=None, use_psd=True, wn_label = 'preprocess.noiseQ_mapmaking.white_noise'): |
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.
Wrap that new arg onto next line. Remove spaces around "=".
I see this arg, as well as use_psd
, aren't included in the docstring. These args are documented elsewhere ... so either copy paste that here, or else reduce duplication and just refer the reader somehow ... "the use_psd and wn_label args are passed through to add_obs; see that docstring".
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'm approving but I think you should address the one issue below.
sotodlib/mapmaking/noise_model.py
Outdated
def check_ready(self): | ||
return self.ivar 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.
This is at odds with the (undocumented) point of base class Nmat.check_ready
, which is designed to raise and exception if the ready
condition is not met. So then calling self.check_ready()
, in self.write()
for example, has no consequence.
I think what you're trying to do might better be achieved by removing this redefinition of check_ready
, and instead making ready
a computed attribute:
@property
def ready(self):
return self.ivar 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.
computed atribute means re-calculated every time I ask for it? That's what I need. If I ask the first time, it should be False since ivar is None, the second time I ask it should be True since ivar will be != 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.
ok I think I have answered my own question
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.
Please rebase/squash on merge! Thanks.
This is not ready yet, I haven't tested either on real data, but I guess people can look at the code in the meantime.