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

Clustering cleanup #704

Merged
merged 6 commits into from
Sep 13, 2024
Merged

Clustering cleanup #704

merged 6 commits into from
Sep 13, 2024

Conversation

jeremykubica
Copy link
Contributor

This PR overhauls how the clustering options and configurations to make them less dependent on the input data. Changes include:

  • The "all" clustering type now uses unnormalized position and velocity (x0, y0, vx, vy) instead of normalized position, velocity magnitude, and velocity angle. This means clustering eps does not change with image size.
  • Drop the normalization option from all searches. The clustering should not depend on image size.
  • Add an explicit "cluster_v_scale" parameter to control the tradeoff of position vs velocity instead of using the search parameter range. This is more flexible and stable over different searches.
  • Rename eps to the more meaningful cluster_eps.
  • Set cluster_eps default to 20.0 to account for the lack of normalization in the searches.
  • Remove broken benchmark for clustering.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@DinoBektesevic DinoBektesevic left a comment

Choose a reason for hiding this comment

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

I think this is worth it just for the name changes, but I've a question regarding the velocity_scale == 0 situation.

src/kbmod/filters/clustering_filters.py Outdated Show resolved Hide resolved
@jeremykubica jeremykubica merged commit 18d255f into main Sep 13, 2024
2 checks passed
@jeremykubica jeremykubica deleted the clustering_cleanup branch September 13, 2024 18:11
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