Skip to content
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-39858: Integrate new CalibrateImageTask with AP pipeline #194

Merged
merged 2 commits into from Aug 21, 2023

Conversation

parejkoj
Copy link
Contributor

No description provided.

@parejkoj parejkoj force-pushed the tickets/DM-39858 branch 3 times, most recently from b165817 to 956b010 Compare August 15, 2023 20:42
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look better, but I'm worried about whether we want to measure the source count metrics using the old code. Would like to discuss before approving.

pipelines/_ingredients/ConversionsCalibrateImage.yaml Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this file should be a diff on Conversions.yaml (or vice versa, given future plans), given that the only tasks that are different are writeSourceTable and consolidateVisitSummary.

pipelines/DarkEnergyCamera/ApVerifyCalibrateImage.yaml Outdated Show resolved Hide resolved
pipelines/_ingredients/ApVerify.yaml Outdated Show resolved Hide resolved
pipelines/_ingredients/ApVerifyCalibrateImage.yaml Outdated Show resolved Hide resolved
Comment on lines 30 to 12
numSciSources:
class: lsst.ip.diffim.metrics.NumberSciSourcesMetricTask
config:
connections.sources: initial_stars_footprints_detector
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that initial_stars does not contain all sources, is this actually still a valid measurement of numSciSources as defined in our specs? Granted, the description that's in there right now is obviously typoed (it would imply fracDiaSourcesToSciSources ≡ 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just chatted with @ebellm about this (he happened to be nearby): it's fine that numSciSources will have a different meaning now (and fracDiaSourcesToSciSources will be quite different, too, and potentially > 1!). We should either fix that description, or rename it to be what the description says. If we are going to report such a value, it would have to come from an afterburner-style detection and measurement catalog, a la RFC-938, which we don't have yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer renaming, just so that the same metric always means the same thing and there's no confusion/ambiguity.

So what exactly are "initial stars", and what is the significance of their number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initial_stars is defined as Catalog of unresolved sources detected on the calibrated exposure. As currently designed, it's the list of isolated S/N>10 psf-like sources that are used for astrometric and photometric calibration. The significance of their number tells you something about how many good isolated sources there are on the science image, but I don't know if that's something we really care about as a metric. What was the purpose of numSciSources as it was implemented here?

If we rename this, numSciSources would be None until we get that afterburner implemented.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the purpose of numSciSources as it was implemented here?

@ebellm is probably the better source for that, but I assumed it's a measure of how crowded the field is (whereas numInitialStars will be minimized for both very sparse and very dense fields?) And presumably the number of DiaSources we expect is proportional to the total number of sources, not just the isolated ones.

If we rename this, numSciSources would be None until we get that afterburner implemented.

Exactly what I had in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be worth spending a few minutes discussing at the Monday AP meeting? "What is the purpose of this metric" is something we should probably have written down in their descriptions (in addition to making sure the description is actually correct!).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The very 🏃 meeting discussion seemed to agree that we don't particularly care about counting the initial stars.

I suppose that means all the "derived" metrics would also be removed? (We really should have had a standalone "number of DiaSources" metric and done the math in Chronograf, but we didn't appreciate that at the time, and it was before DMTN-085 made that the policy.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per AP meeting discussion today, I've removed all four of these "sci sources" metrics.

Yeah, we probably should have a "number of diaSources" metric. It may be worthwhile to have "number of calibration sources" as well (i.e. the output of this new task). I think that's work for another ticket, though.

@parejkoj parejkoj force-pushed the tickets/DM-39858 branch 2 times, most recently from 2a09ca9 to d2b2bb6 Compare August 21, 2023 22:05
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, one last comment about the imports between the old-style and *CalibrateImage.yaml files.

These allow running ap_verify.py with the new task as a separate run,
so that we can directly compare the new and old versions.
@parejkoj parejkoj merged commit fcfaf65 into main Aug 21, 2023
2 checks passed
@parejkoj parejkoj deleted the tickets/DM-39858 branch August 21, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants