-
Notifications
You must be signed in to change notification settings - Fork 3
DM-48045: Create template metrics and template QC process for Prompt Processing. #408
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
Conversation
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.
Code looks good, but I've got a few thoughts on how we structure this. Mostly along the lines of "can we combine this with that"?
I'm willing to be persuaded either way, so don't feel I'm saying we have to go one way or the other. Let me know your thoughts on what I've said.
Oh, and you'll probably have seen that it's failing GitHub checks on black formatting and isort.
pipelines/deepCoaddQualityCore.yaml
Outdated
| @@ -0,0 +1,22 @@ | |||
| description: | | |||
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.
Do we really want to add another two pipelines on top of coaddQualityCore.yaml?
Could we instead just add this to coaddQualityCore.yaml, using either deep or template as the default connections.coaddType, then override that in either drp_pipe or ap_pipe (depending on which we choose - deep or template).
I think I'd prefer for the default here to be deep, since that's what's generally found in coaddQualityCore.yaml. We'd just make sure that it wasn't run as part of drp_pipe (if we didn't want it to be).
I saw the chat on dm-science-pipelines about overriding coaddName, but I don't think that's referring to this - I think that's specifically referring to doing all the coaddQualityCore tasks with template as well as deep.
| @@ -0,0 +1,22 @@ | |||
| description: | | |||
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.
See above comment.
|
|
||
| plt.subplots_adjust(hspace=0, wspace=0) | ||
|
|
||
| patch_counter = 90 # The top left corner of a tract is patch 90 |
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.
Isn't hard-coding this risky? Just thinking that other skymaps may not have the same number of patches per tract.
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.
This class should be using the skymap tractInfo. See PerTractPropertyMapPlot for an example.
| yield self.key_is_target_star, Vector | ||
|
|
||
|
|
||
| class UniqueAction(VectorAction): |
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 this used? I can't find anywhere in the PR where this is used. Was it added, then not needed, or have I missed something?
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 not used and I ended up removing it from the PR. :)
|
|
||
| def __call__(self, data: KeyedData, **kwargs) -> Vector: | ||
| patches: Optional[tuple[str, ...]] | ||
| match kwargs: |
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.
While I like what this is doing grammatically (patch vs. patches 😄 ), and I see that we do the same for band/bands, I wonder whether we should just keep it simple and have just patches=None as a keyword so that it's easier to see at a glance.
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.
Dan also added a PatchSelector so I deferred to his and removed mine from the PR. :)
| self.produce.metric.units[f"{name}_stdev"] = "" | ||
|
|
||
|
|
||
| class CoaddQualityPlot(AnalysisTool): |
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.
Why not combine the metric and the plot generation into a single atool? Or do you want to keep them separate so that plots are only generated when needed, but metrics are made for everything?
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.
The plots can take a while to generate depending on how much data so I kept 'em separate. When I run the QC metrics afterburner pipeline, I skip the plot generation unless it's specifically asked for.
|
|
||
| __all__ = ( | ||
| "CoaddDepthSummaryPlotConfig", | ||
| "CoaddDepthSummaryPlotTask", |
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.
Similarly, do we want to combine these two tasks into one? Or keep them separate so that we don't have to make plots if all we want are metrics?
| return cast(Vector, mask) | ||
|
|
||
|
|
||
| class PatchSelector(VectorAction): |
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.
FYI I added a PatchSelector recently, but it doesn't look like you actually need it.
| while patch_counter >= 0: | ||
| for n in range(10): # column index | ||
| ax = plt.subplot(10, 10, m + 1) # there are 10x10 patches per tract | ||
| patchSelector = PatchSelector(vectorKey='patch', patches=[patch_counter]) |
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.
There's no point making a ConfigurableAction inside a call method where it can't actually be reconfigured in a pipeline. You already put patch and band in the schema so you may as well just do patch_mask = data["patch"] == patch_counter, and same for band_mask.
0748c64 to
1948c9c
Compare
|
Some unsolicited suggestions re. pipelines
|
91bddf5 to
e277a29
Compare
3669189 to
36baf53
Compare
cbab6b9 to
d935f0a
Compare
jrmullaney
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.
Looks good.
I've left a couple of comments about naming outputs, and a request to add an example figure for your new plot action (I've only been made aware of these example figures since I last reviewed this ticket).
Approved.
| ------- | ||
| fig : `~matplotlib.figure.Figure` | ||
| The resulting figure. | ||
| """ |
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.
Could you add an example figure here, please? Sophie Reed pointed this out to me when I recently made a new plot action. An example is here (but most other plot actions have them, too). It took me a while to figure out where I needed to put the plot - it goes in doc/_static/analysis_tools/ in this repo, then the doc generator finds it when it builds the docs for the website. Pretty nifty!
| class CoaddInputCount(AnalysisTool): | ||
| """skyPlot and associated metrics indicating the number | ||
| of exposures that have gone into creating a coadd. | ||
| """Coadd-wide metrics pertaining to how many exposures have gone into |
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.
Could you change this to "Tract-wide" instead, to make it clear that it's on a per-tract basis, rather than a per-patch basis (at least, I think that's the case)?
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 don't actually know much about this metric. The comment change was mostly to alert folks that this does not work for template_coadds. I'll put the comment changes into their own commit and then ask for clarity on Slack. Thanks for catching that!
| ) | ||
|
|
||
|
|
||
| class CoaddDepthSummaryAnalysisTask(AnalysisPipelineTask): |
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.
Does this need to be an AnalysisPipelineTask? It seems it should be a just a regular PipelineTask. Does it use any of the AnalysisPipelineTask infrastructure?
| dimensions=("tract", "skymap"), | ||
| defaultTemplates={ | ||
| "coaddType": "", # set as either deep or template in the pipeline | ||
| "outputName": "coadd_depth_table", |
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.
Do you want the outputName to include the type of coadd?
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 love to figure out how to do that! I couldn't manage any sort of "outputName": "{coaddType}_coadd_depth_table" that actually worked. If you know the trick, I'd love to update these. :)
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.
For this specific task, you can just turn it into a regular PipelineTask, in which case you can just do:
statTable = cT.Output(
doc="Table with resulting n_image based depth statistics.",
name="{coaddType}_coadd_depth_table",
storageClass="ArrowAstropy",
dimensions=("tract", "skymap"),
)
It doesn't need to be an AnalysisPipelineTask. You can then get rid of outputName in the defaultTemplates.
| dimensions=("tract", "skymap"), | ||
| defaultTemplates={ | ||
| "coaddType": "", | ||
| "outputName": "n_image", |
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.
Similarly, should this include the coadd type?
| class CoaddDepthTableTractAnalysisConnections( | ||
| AnalysisBaseConnections, | ||
| dimensions=("tract", "skymap"), | ||
| defaultTemplates={"inputName": "coadd_depth_table", "outputName": "coadd_depth_summary"}, |
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.
Should this include the coadd type?
f9afff1 to
48d2fb4
Compare
48d2fb4 to
bfa3cb5
Compare
No description provided.