-
Notifications
You must be signed in to change notification settings - Fork 19
Allow pipeline to take input full axis manager #1346
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
try: | ||
error, outputs_grp_init, _, aman = pp_util.preproc_or_load_group(obs_id, configs_init, | ||
dets=dets, logger=logger) | ||
error, outputs_grp_init, _, aman, full = pp_util.preproc_or_load_group(obs_id, configs_init, |
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 would suggest to rename full
to full_aman
here. It will help in the future.
as_completed_callable: Callable, | ||
configs: str, | ||
query: Optional[str] = None, | ||
query: Optional[str] = '', |
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 you want the default value to be a string, then Optional
is not necessary.
if return_full: | ||
return error[0], [error[1], error[2]], [error[1], error[2]], None, None | ||
else: | ||
return error[0], [error[1], error[2]], [error[1], error[2]], 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.
May I suggest to not change the signature of the return value. Instead if return_full
is set and there is a value different from None
, you return that.
Essentially the signature here would be error[0], [error[1], error[2]], [error[1], error[2]], None, None
regardless of the value of return_full
, and in 1018 it would be:
if return_full:
return error, [obs_id, dets], outputs_proc, aman, proc_aman
else:
return error, [obs_id, dets], outputs_proc, aman, None
Then in the call you can do o1, o2, o3, _ = preproc_or_load_group(...)
when you do not need the output.
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.
You seem to have made the number of returned arguments 5 everywhere else but still have the option of having it be 4 here if return_full = False
is there a reason you left that in?
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.
Ah looks like I missed this one somehow. Will fix.
Thank you for the comments. I think I've updated things to match them. Also did a small fix in I've updated the mapmaker call to |
if return_full: | ||
return error[0], [error[1], error[2]], [error[1], error[2]], None, None | ||
else: | ||
return error[0], [error[1], error[2]], [error[1], error[2]], 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.
You seem to have made the number of returned arguments 5 everywhere else but still have the option of having it be 4 here if return_full = False
is there a reason you left that in?
Merge messed up some things and realized there's another case I need to handle so will move to draft until done cleaning this up. |
Intended to address #1323. Updates the
Pipeline
class inpcore
to acceptfull
as an input. Defaults to None such that the previous behavior is maintained. This allows for multilayer pipelines to preserve the originaldetector
andsamps
counts. The loading functionmultilayer_load_and_preprocess
now loops through both of theinit
andproc
pipeline restrictions now to propagate all of the cuts.Also includes one small fix for the
query
input forpreprocess_tod
andmultilayer_preprocess_tod
. The default was originallyNone
, but this doesn't work for the file checking line here:sotodlib/sotodlib/site_pipeline/util.py
Line 379 in 9754e46