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

Debug DirectedSphereExclusion #155

Merged
merged 2 commits into from
Aug 23, 2023
Merged

Debug DirectedSphereExclusion #155

merged 2 commits into from
Aug 23, 2023

Conversation

marco-2023
Copy link
Collaborator

The select_from_cluster method did not use the data from the cluster labels. Because of this, the algorithm used the whole data instead of the samples of the cluster from which the selection should occur. The algorithm then failed if cluster labels were used.

This PR fixes #154

@codecov
Copy link

codecov bot commented Aug 9, 2023

Codecov Report

Merging #155 (eefe251) into main (d0b9c79) will decrease coverage by 0.30%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
- Coverage   40.64%   40.35%   -0.30%     
==========================================
  Files           8        8              
  Lines         556      560       +4     
==========================================
  Hits          226      226              
- Misses        330      334       +4     
Files Changed Coverage Δ
DiverseSelector/methods/dissimilarity.py 18.60% <0.00%> (-0.45%) ⬇️
DiverseSelector/methods/partition.py 11.25% <14.28%> (-0.10%) ⬇️

@marco-2023
Copy link
Collaborator Author

I added the same fix to the OptiSim class.

@FanwangM
Copy link
Collaborator

Can you add one test for this case? Thank you. @marco-2023

@marco-2023
Copy link
Collaborator Author

Hello @FanwangM, I did those minor fixes because otherwise, the Jupyter Notebook did not run. I think Ali is working on fixing the OptiSim module so I tried not to change too many things (to avoid getting in his way). Should I still implement a test for this?

@FanwangM
Copy link
Collaborator

Hi @marco-2023. Yes, once you have a small test for the directed sphere exclusion, I will merge this PR. Thank you.

The select_from_cluster method did not use the data from the
cluster labels. Because of this, the algorithm used the whole
data instead of the samples of the cluster from which the
selection should occur. The algorithm then failed if cluster
labels were used.
The select_from_cluster method did not use the data from the
cluster labels. Because of this, the algorithm used the whole
data instead of the samples of the cluster from which the
selection should occur.
@FanwangM FanwangM force-pushed the debug_DirectedSphereExclusion branch from 8771a76 to eefe251 Compare August 23, 2023 02:03
@FanwangM FanwangM merged commit bc4c2fd into main Aug 23, 2023
4 of 10 checks passed
JackyZzZz pushed a commit to JackyZzZz/Selector that referenced this pull request Jun 14, 2024
…lusion

Fix the problem where `select_from_cluster` method did not use the data from the cluster labels
FanwangM added a commit that referenced this pull request Jul 2, 2024
Fix the problem where `select_from_cluster` method did not use the data from the cluster labels
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.

optimize_radius not using information of clusters
2 participants