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

Yanza/custom useful clusters #7

Merged
merged 20 commits into from
Nov 18, 2024
Merged

Conversation

yanzastro
Copy link
Contributor

This PR adds a parameter to the somoclu summarizer so that the user can customize which clusters are calibrated. This function is useful when we do quality control by ignoring "bad clusters".

Change Description

  • My PR includes a link to the issue that I am addressing

Solution Description

Code Quality

  • X I have read the Contribution Guide
  • X My code follows the code style of this project
  • X My code builds (or compiles) cleanly without any errors or warnings
  • XMy code contains relevant comments and necessary documentation

@yanzastro yanzastro requested a review from joselotl August 4, 2023 17:44
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c882a58) 100.00% compared to head (72517a5) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #7   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          319       322    +3     
=========================================
+ Hits           319       322    +3     
Files Coverage Δ
src/rail/estimation/algos/somoclu_som.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aimalz aimalz marked this pull request as draft August 4, 2023 22:03
@aimalz
Copy link
Contributor

aimalz commented Aug 4, 2023

I changed this PR to a draft so @yanzastro can separate out the good/bad cluster definition as a Classifier, allowing it to be used as a preprocessing stage for any summarizer.

@aimalz aimalz self-requested a review August 4, 2023 22:06
@yanzastro yanzastro marked this pull request as ready for review November 6, 2023 10:31
Copy link
Contributor

@joselotl joselotl left a comment

Choose a reason for hiding this comment

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

Everything looks fine. Sorry for taking so long to do the review

ztq1996 pushed a commit that referenced this pull request May 1, 2024
missed one of two things I needed to change to fix the nan issue
@aimalz
Copy link
Contributor

aimalz commented Jun 3, 2024

Thanks for your patience with my review! This looks fine, but I noticed the code for defining bad clusters is still bundled in with the Summarizer rather than separated out as a Classifier that could be used more generically. Is there a reason for this?

@yanzastro yanzastro merged commit b455da8 into main Nov 18, 2024
4 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.

3 participants