This repository was archived by the owner on Jan 15, 2026. It is now read-only.
Moving fps and cur to scikit-cosmo version with backwards compatibility#347
Merged
Conversation
max-veit
force-pushed
the
feat/remove_fps_and_cur
branch
from
78a7ab1 to
79be454
Compare
Luthaf
reviewed
Aug 23, 2021
Contributor
|
This PR looks very goo overall, the new code is much simpler! |
Luthaf
reviewed
Aug 24, 2021
* Adding skcosmo to requirements * Make scikit-cosmo an optional dependency (and add to CI)
* Making filter work for tests/python/python_representation_calculator_test.py Fleshing out tests * Blacking tests * Make some asserts more specific * Reduce sparse points so i-PI zundel example runs faster (it was timing out in the CI. The potential still gives stable MD.)
* Try re-adding functionality to plot FPS Hausdorff distances * maek lnit lank time lake mint late mink :( * Somewhat tangential docs update * Apply quick suggestions Co-authored-by: Guillaume Fraux <guillaume.fraux@epfl.ch> * More extensive changes from review * Update feature sparsification notebook for modified API * Update doc for Filter and some more minor doc and cosmoetic updates * Remove filter mode assert (validity check is done in superclass) * Make wrong-selection-mode error handling more robust and friendly * Add a check on size of the returned SparsePoints * Update the global-sample-mode selection to return a SparsePoints (it's ordered by species, but that's usually OK) * Fix checks of error messages The previous constuction wasn't actually getting to the assertEqual(cm.message, ...) bits because the exception broke it out of that code block first Also fix the actual error messages following the checks
The reference implementation is now in scikit-cosmo
Now they no longer need cryptic helper functions and are potentially more useful outside the Filter class Update example notebook
since it's an optional dependency
max-veit
force-pushed
the
feat/remove_fps_and_cur
branch
from
4bb05b2 to
8c63bef
Compare
also fix some merge detritus
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
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.
The FPSFilter and CURFilter classes currently implement functionality that is better handled (in some cases, already implemented) in scikit-cosmo. Therefore, we are refactoring these classes so that these tasks can be handled by skcosmo instead.
This refactoring includes removing some code duplication that was present in these versions (and caused e.g. #333 ) by making both filters inherit from a common base class,
Filter. The newCURFilterandFPSFilterclasses behave exactly as the old ones, so there should be little to no change necessary for end users.Tasks before review:
formatted correctly (ask @max-veit if you need help with this task).
explain the feature and its usage in plain English
make linton the project, ensure it passesown branch
nbstripout