-
Notifications
You must be signed in to change notification settings - Fork 19
Add loader and det cal functionality to load in bandpass info using bg #1206
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
base: master
Are you sure you want to change the base?
Conversation
if cal.bg in bgs['lb']: | ||
cal.bandpass = band_str[tube_flavor]['lb'] | ||
elif cal.bg in bgs['hb']: | ||
cal.bandpass = band_str[tube_flavor]['hb'] |
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.
If referencing globals I think this needs to be capitalized variable names here.
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.
Yeah sorry, missed this earlier
I think I like this setup but will leave it to @mhasself to approve the additional loader option. To deploy this we'll need to update the existing det cal databases correct? I believe these are the instructions for setting up to deploy the update. https://simonsobs.atlassian.net/wiki/spaces/PRO/pages/306249767/Site+Pipeline+Metadata+Logs |
Sorry for the delay. I'll review this soon but one thing to do immediately is add tests for |
Is there any reason you can think of (and I'll have a go, too) to not include this functionality directly in ResultSetHdfLoader? Seems backwards compatible. |
My reasons were a combination of:
But these aren't that good. |
@mhasself Tommy has added tests, could we get a review here? |
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.
Focusing on the metadata changes -- I think it would be good to just add this functionality directly to the base class. This is because the "load_fields" is already implemented on AxisManager loader, with similar semantics (which we can make more similar, in the future) -- and you've taken advantage of the fact that "load_fields", in metadata definitions, is propagated through to the loader. Which is great.
Once that is done there are a couple of documentation fixes -- i.e. remove the "this only works on AxisManager" warning; and mention instead that the ResultSet loader also lets you rename fields. The syntax should be included somehow in readthedocs -- I should be able to reconstruct it starting from this autodocced class.
Thanks for the tests.
sotodlib/preprocess/processes.py
Outdated
@@ -1343,6 +1343,7 @@ class PCARelCal(_Preprocess): | |||
def __init__(self, step_cfgs): | |||
self.signal = step_cfgs.get('signal', 'signal') | |||
self.run = step_cfgs.get('pca_run', 'run1') | |||
self.bandpass = step_cfgs.get('bandpass', 'wafer.bandpass') |
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 think it would be better to call this "bandpass_key"
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.
Are you saying to use 'self.bandpass_key' as the class variable name or to use 'bandpass_key' as the config parameter name? Or both?
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 going to guess you mean the config parameter name, that is much more visible
tests/test_context.py
Outdated
@@ -311,6 +311,19 @@ def test_120_load_fields(self): | |||
self.assertCountEqual(tod.ondisk._fields.keys(), ['disk1', 'subaman']) | |||
self.assertCountEqual(tod.ondisk.subaman._fields.keys(), ['disk2']) | |||
|
|||
def test_130_load_fields(self): |
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.
"load_fields_resultset" would distinguish this from test_120 more clearly.
Adds custom functionality to be able to load in bandpass information using the
det_cal
bias group information. The pull request does a combination of:update_det_cal.py
to calculate and save bandpass information inside the ManifestDB from the bias group.processes.py
to allow a fieldname for the bandpass as input rather than using the hard-codedwafer.bandpass
With the custom loader we can load up the
bandpass
field from the newdet_cal
intoaman.det_info
using e.g. a field namedets:det_cal.bandpass
. The rest of the det cal information can be then loaded intoaman.det_cal
with a separate context entry using the same database.Here's an example context file snippet which uses the loader to load
det_cal
information. This assumes that thedet_cal
database has the bandpass information stored in the fieldbandpass
.In this example, the custom loader first loads in
bandpass
information fromdet_cal
intoaman.det_info.det_cal.bandpass
by specifying only to read indets:readout_id
andbandpass
from our det cal database, as well as preprendingdets:detcal.
to thebandpass
field so that it properly can be read intodet_info
.Then, the default loader reads all of the
det_cal
fields intoaman.det_cal
.Note that this has the slightly inefficient side effect that
bandpass
is loaded intodet_cal
but also insidedet_info.det_cal
in the same axismanager. This can be fixed with the current functionality if the user uses the custom loader and inputs every field besidesbandpass
to load, although this would be a fairly verbose context file. Or, if you want I can modify the loader code to add something like the inputdrop_fields
toloader.from_loadspec
but I figured simpler and fewer changes to the loader were better.