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

ZTF Alert FP - deduplication logic, flux to mag space #261

Merged
merged 6 commits into from
Dec 4, 2023

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Nov 10, 2023

This PR:

  • add the deduplication logic folks decided to use, i.e. when we have overlapping FP datapoints, replace existing ones only if the new alert is brighter (which means, the position should be more accurate).
  • add mag space information when the SNR is above 0. That way, filters that need the mag can trust it or not trust it by enforcing their own SNR cuts.
  • ensure that only brand new ZTF objects start accumulating forced photometry. That way, we either don't have FP, or we have the full history for that object. Otherwise, filters relying on FP could yield unpredictable results.
  • add tests for the deduplication

TODO: add tests for the mag space data maybe?

@mcoughlin mcoughlin changed the title WIP: ZTF Alert FP - deduplication logic, flux to mag space ZTF Alert FP - deduplication logic, flux to mag space Nov 29, 2023
Copy link
Collaborator

@mcoughlin mcoughlin left a comment

Choose a reason for hiding this comment

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

a few thoughts / minor doc edits.

kowalski/alert_brokers/alert_broker.py Show resolved Hide resolved
kowalski/alert_brokers/alert_broker.py Outdated Show resolved Hide resolved
kowalski/alert_brokers/alert_broker_ztf.py Show resolved Hide resolved
kowalski/alert_brokers/alert_broker_ztf.py Show resolved Hide resolved
kowalski/alert_brokers/alert_broker_ztf.py Outdated Show resolved Hide resolved
kowalski/ingesters/ingester.py Show resolved Hide resolved
kowalski/tests/test_alert_broker_ztf.py Outdated Show resolved Hide resolved
kowalski/tests/test_alert_broker_ztf.py Outdated Show resolved Hide resolved
kowalski/tests/test_alert_broker_ztf.py Show resolved Hide resolved
kowalski/tools/kafka_stream.py Show resolved Hide resolved
@Theodlz
Copy link
Collaborator Author

Theodlz commented Nov 29, 2023

Thanks a lot for the review @mcoughlin (and sorry for the stupid english mistakes, you make one of them and then copilot propagates that error as much as possible yikes).

@Theodlz Theodlz merged commit a100fb1 into skyportal:main Dec 4, 2023
2 checks passed
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.

2 participants