Allowing FPS to take numpy array of ints as initialize parameter #225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PicoCentauri
reviewed
May 3, 2024
src/skmatter/_selection.py
Outdated
| self.selected_idx_[i] = val | ||
| self._update_post_selection(X, y, self.selected_idx_[i]) | ||
| else: | ||
| raise ValueError("Initialize parameter must contain only int") |
There was a problem hiding this comment.
Would you mind adding a test for this error raise?
You can take a look how to test that in the pytest docs or find another example in our test suite
There was a problem hiding this comment.
Should be fixed now. Added the unit tests I wrote in test_feature_simple_fps to test_sample_simple_fps. Let me know if this looks correct!
PicoCentauri
reviewed
May 6, 2024
src/skmatter/_selection.py
Outdated
| @@ -1038,7 +1038,14 @@ def _init_greedy_search(self, X, y, n_to_select): | |||
| self.hausdorff_ = np.full(X.shape[self._axis], np.inf) | |||
| self.hausdorff_at_select_ = np.full(X.shape[self._axis], np.inf) | |||
|
|
|||
| if self.initialize == "random": | |||
| if isinstance(self.initialize, np.ndarray): | |||
There was a problem hiding this comment.
Shouldn't these operattions also be performed for a List and not only for a numpy array? Something like
if isinstance(self.initialize, np.ndarray) or isinstance(self.initialize, list):Even though there is probably a shorter way to check of an instance belongs to either one of two classes.
There was a problem hiding this comment.
Yes. Let me know what you think about the new changes I made.
Bump supported and testes Python versions
|
Updated the changelog! |
PicoCentauri
approved these changes
May 14, 2024
rosecers
approved these changes
May 16, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hello!
This pull request is my attempt to address issue #145. With these changes, FPS should be able to accept a numpy array of ints as an input. I wrote a few tests as well to ensure the updated code is functioning properly. I'd love to hear any feedback you may have!
Contributor (creator of PR) checklist
For Reviewer
📚 Documentation preview 📚: https://scikit-matter--225.org.readthedocs.build/en/225/