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
DM-40441: Rewrite HiPS custom QG builder to inherit from new base class. #913
Conversation
This avoids using deprecated interfaces like Pipeline.toExpandedPipeline while letting the base class handle a lot of details.
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.
Great to see registry
disappearing as a parameter.
"""A custom a `lsst.pipe.base.QuantumGraphBuilder` for running | ||
`HighResolutionHipsTask` only. | ||
|
||
This is a temporary workaround for incomplete butler query support for |
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.
Is there a ticket number for the real solution?
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.
No, and I'm not sure how I want to do it yet; there are a few options but none seem terribly close. Might be best to drop the "temporary" instead.
@@ -596,24 +606,26 @@ def build_quantum_graph( | |||
where_terms.append(f"({common_skypix_name} >= cpx{n}a AND {common_skypix_name} <= cpx{n}b)") | |||
bind[f"cpx{n}a"] = begin | |||
bind[f"cpx{n}b"] = stop | |||
if where is None: | |||
if not self.where: |
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 know this is pre-existing but you are always calculating the join of where_terms.
where = " OR ".join(where_terms)
if self.where:
where = f"({self.where}) AND ({where})"
might be better?
hpx_output_ranges = hpx_pixel_ranges.scaled(4**(config.hips_order - hpx_pixelization.level)) | ||
output_data_ids = [] | ||
hpx_output_ranges = hpx_pixel_ranges.scaled( | ||
4**(task_node.config.hips_order - hpx_pixelization.level) |
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.
Is there a note anywhere explaining where the 4 comes from? (it turns up in a couple of places).
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.
It's the number of healpixels at level N+1 in each healpixel at level N. I'll add a comment somewhere when I get a chance.
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.
Apologies; I just merged this and only then realized I'd forgotten about the comments here while testing my changes in other packages. I think I've got another deprecation/removal ticket coming up in pipe_tasks shortly and will do them there.
"""A custom a `lsst.pipe.base.QuantumGraphBuilder` for running | ||
`HighResolutionHipsTask` only. | ||
|
||
This is a temporary workaround for incomplete butler query support for |
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.
No, and I'm not sure how I want to do it yet; there are a few options but none seem terribly close. Might be best to drop the "temporary" instead.
hpx_output_ranges = hpx_pixel_ranges.scaled(4**(config.hips_order - hpx_pixelization.level)) | ||
output_data_ids = [] | ||
hpx_output_ranges = hpx_pixel_ranges.scaled( | ||
4**(task_node.config.hips_order - hpx_pixelization.level) |
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.
It's the number of healpixels at level N+1 in each healpixel at level N. I'll add a comment somewhere when I get a chance.
This avoids using deprecated interfaces like
Pipeline.toExpandedPipeline
while letting the base class handle a lot of details.