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-27032: Create deblended source metric #512

Merged
merged 4 commits into from Jun 18, 2021
Merged

Conversation

kfindeisen
Copy link
Member

This PR implements two new deblending-related metrics defined in lsst/verify_metrics#27.

Copy link
Contributor

@mrawls mrawls left a comment

Choose a reason for hiding this comment

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

This looks very good! Please ensure you've fully digested the Community post pertaining to how the children, parents, primary, inner regions, etc. are all related. In line with this, you might consider adding tests to check, e.g., if the number of one kind of source is less/greater than the number of a different kind of source.

python/lsst/pipe/tasks/metrics.py Show resolved Hide resolved
python/lsst/pipe/tasks/metrics.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/metrics.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/metrics.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/metrics.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/metrics.py Outdated Show resolved Hide resolved
tests/test_metrics.py Show resolved Hide resolved
meas = result.measurement

self.assertEqual(meas.metric_name, self.METRIC_NAME)
assert_quantity_allclose(meas.quantity, u.Quantity(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL about this handy assert_quantity_allclose. It feels a bit like overkill here where there aren't really quantities with units and values in the dummy catalog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I remove it? I think it was more a "better safe than sorry" thing...

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it hurts anything, I just spent some time trying to figure out where the quantities you were comparing were hiding, is all :) I like the idea that some day, our catalogs may indeed have such attributes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I'm confused. I'm comparing the Quantity output by the metric task to a reference value; where do the input catalogs come in?

I thought your "overkill" comment was about not using exact equality for values that started life as integers...

Copy link
Contributor

Choose a reason for hiding this comment

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

I wrote "catalogs" but really meant "thing the metric is computing" (which may or may not originate from a catalog, and may or may not have units attached to it). Sorry for the confusion.

tests/test_metrics.py Show resolved Hide resolved
These tasks could probably be factored further, since the only
difference is in the criteria used for filtering sources, but handling
columns in a general way would be nontrivial and should probably await
the addition of more varied metrics.
@kfindeisen kfindeisen merged commit 9e5eb3b into master Jun 18, 2021
@kfindeisen kfindeisen deleted the tickets/DM-27032 branch June 18, 2021 22:42
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.

None yet

2 participants