Skip to content

Conversation

mmccrackan
Copy link
Contributor

Intended to address #1323. Updates the Pipeline class in pcore to accept full as an input. Defaults to None such that the previous behavior is maintained. This allows for multilayer pipelines to preserve the original detector and samps counts. The loading function multilayer_load_and_preprocess now loops through both of the init and proc pipeline restrictions now to propagate all of the cuts.

Also includes one small fix for the query input for preprocess_tod and multilayer_preprocess_tod. The default was originally None, but this doesn't work for the file checking line here:

with open(query, "r") as fname:
so it has been replaced with an empty string.

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,
Copy link
Member

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] = '',
Copy link
Member

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.

Comment on lines 900 to 903
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
Copy link
Member

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@mmccrackan
Copy link
Contributor Author

Thank you for the comments. I think I've updated things to match them. Also did a small fix in preproc_or_load group, which now copies proc_aman after the dependent pipeline has run so it can maintain all first and second layer fields with the full det and samps count. The copied proc_aman has the first layer processes removed, is saved, and then merged into aman, where it is restricted.

I've updated the mapmaker call to preproc_or_load_group to handle the new output field, but have not tested it yet.

Comment on lines 900 to 903
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
Copy link
Contributor

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?

@mmccrackan mmccrackan requested a review from chervias September 12, 2025 15:26
@mmccrackan mmccrackan marked this pull request as draft September 16, 2025 21:28
@mmccrackan
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants