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.

Sign up for GitHub

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

Jump to bottom

DM-43402: Add option to turn off forced measurements on the science and differe… #198

Merged
merged 1 commit into from Mar 20, 2024
Merged

DM-43402: Add option to turn off forced measurements on the science and differe… #198

merged 1 commit into from Mar 20, 2024

Conversation

isullivan
Copy link
Contributor

…nce images

kfindeisen
kfindeisen previously requested changes Mar 20, 2024
View reviewed changes
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.

The concept seems sound enough, but I don't think this code was actually tested, and will break (either when calling Apdb.store or alertPackager.run) if we try to run it...

python/lsst/ap/association/diaPipe.py Outdated Show resolved Hide resolved
python/lsst/ap/association/diaPipe.py Outdated Show resolved Hide resolved
@kfindeisen
Copy link
Member

With the obvious issues fixed, packageAlerts chokes on a schema mismatch. Consulting with @bsmartradio...

parejkoj
parejkoj approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

One spacing cleanup, otherwise my same concern as the earlier ticket about this (that we don't have a way to really test it here), but I won't stop that from merging it, since this is just a stopgap.

python/lsst/ap/association/diaPipe.py Outdated Show resolved Hide resolved
@kfindeisen kfindeisen mentioned this pull request Mar 20, 2024
Merged
@kfindeisen kfindeisen dismissed their stale review March 20, 2024 21:53

Superseded by @parejkoj's review.

@isullivan @kfindeisen
Add option to turn off forced measurements.
fdfa9b3 
These measurements are done on the science and difference images, but
the performance scales badly as the APDB gets larger.
@kfindeisen kfindeisen merged commit 6179bb6 into main Mar 20, 2024
2 checks passed
@kfindeisen kfindeisen deleted the tickets/DM-43402 branch March 20, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfindeisen kfindeisen kfindeisen left review comments

@parejkoj parejkoj parejkoj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@isullivan @kfindeisen @parejkoj