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

Allow large blends. Add error fields #218

Merged
merged 25 commits into from
Aug 5, 2024
Merged

Allow large blends. Add error fields #218

merged 25 commits into from
Aug 5, 2024

Conversation

esheldon
Copy link
Owner

  • set the deblend config to allow large blends
  • set afw_table.CoordKey.addErrorFields(schema) to avoid getting spammed by warnings

This deblending change is important. It should result in a lot more detections in blended regions, and it potentially requires a full retesting.

We might be happy retesting in "no warp" mode, direct to coadd simulations, which would speed it up by a factor of perhaps 2.5

This avoids getting spammed by warnings in new stacks
See if this fixes error in CI

AttributeError: type object 'lsst.afw.table._table.CoordKey' has no
attribute 'addErrorFields'
@esheldon
Copy link
Owner Author

It seems detection may have changed, either due to stack or something we did. Previously objects were detected in zero weight (inf variance) regions but not now.

This test test_lsst_zero_weights might need to change to look for one less detection.

The defaults changed and broke things, so set everything

Add test case where BRIGHT is set and inf var
This fix was put into ngmix back in Oct 2023
we added a new sum for AM that needs to be skipped
for this
@esheldon
Copy link
Owner Author

The lsst-tests metacal exposures test will fail until the new ngmix shows up on conda forge

@esheldon esheldon requested a review from beckermr June 28, 2024 13:10
@esheldon
Copy link
Owner Author

esheldon commented Jun 28, 2024

summary of additional changes

  • the change to detection was in the stack, defaults in config changed
  • set all detection config values explicitly
  • make code for metacal images consistent with ngmix (bug fix)
  • make sure no detection in areas marked BRIGHT
  • deal with extra sum field in AM, added for rho4

@esheldon
Copy link
Owner Author

esheldon commented Aug 5, 2024

@beckermr can you have a look at this one?

metadetect/lsst/util.py Outdated Show resolved Hide resolved
metadetect/lsst/util.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Some minor nits and questions.

@beckermr
Copy link
Collaborator

beckermr commented Aug 5, 2024

Shall we merge @esheldon?

@esheldon esheldon merged commit a9310d9 into master Aug 5, 2024
6 checks passed
@esheldon esheldon deleted the add-error-fields branch August 5, 2024 22:23
keys = config.toDict().keys()

for key in keys:
assert key in expected_keys, f'found unexpected config key {key}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what this is meant to achieve. It will fail the moment we add a new field to this config, but won't if we remove a config. DM guarantees to deprecate a field before removing and not remove a field until the next official release. DM makes no such guarantee when it comes to adding new fields.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The point is that if something changes, we need to understand what that change is and ensure it works. We shouldn't simply let things float.

@arunkannawadi
Copy link
Contributor

arunkannawadi commented Aug 28, 2024 via email

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