-
Notifications
You must be signed in to change notification settings - Fork 21
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. |
2497d99 to
07472fc
Compare
|
Okay this ended up being more complicated than expected since all the loading functions needed to change too, but I think it is probably ready for review again. |
msilvafe
left a comment
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.
A couple inline comments to respond to and want to know how you tested this before approving.
Intended to address #1323. Updates the
Pipelineclass inpcoreto acceptfullas an input. Defaults to None such that the previous behavior is maintained. This allows for multilayer pipelines to preserve the originaldetectorandsampscounts. The loading functionmultilayer_load_and_preprocessnow loops through both of theinitandprocpipeline restrictions now to propagate all of the cuts.Also includes one small fix for the
queryinput forpreprocess_todandmultilayer_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