Added GCH functionality. #140
Conversation
rosecers
left a comment
rosecers
left a comment
There was a problem hiding this comment.
Looking good so far! Left some comments.
victorprincipe
requested review from
Luthaf and
rosecers
and removed request for
agoscinski
agoscinski
left a comment
agoscinski
left a comment
There was a problem hiding this comment.
Please merge first #146, then rebase this
|
for information: the changes in the .github/workflows/tests.yml and pyproject.toml will be gone after rebase, they are in #146 I can do the changes in the setup.cfg in a separate commit before merging. They are required because of |
setup.cfg
Outdated
| install_requires = ["numpy", | ||
| "scikit-learn>=0.24.0"] |
There was a problem hiding this comment.
I typically use this syntax, does it not work?
| install_requires = ["numpy", | |
| "scikit-learn>=0.24.0"] | |
| install_requires = | |
| numpy | |
| scikit-learn>=0.24.0 |
rosecers
left a comment
rosecers
left a comment
There was a problem hiding this comment.
In general looks good, but would love to hear the reasoning behind the templating before approval.
rosecers
left a comment
rosecers
left a comment
There was a problem hiding this comment.
Looks good! Please separate off the non-relevant changes and we can merge.
|
rebased. I have made a backup and sent to @victorprincipe in case I screwed something up |
|
Weren't there tests in here as well? Please make sure that there is proper code coverage. |
My bad, forgot to add them on rebase. Now they are added. Will request review as soon test run through. |
|
@rosecers okay seems to work now |
|
We would squash everything to one commit |
skcosmo/sample_selection/_base.py
Outdated
| features (i.e. those not used for the convex hull | ||
| construction) | ||
| vertices_idx_ : numpy.ndarray |
There was a problem hiding this comment.
Why do we deviate from the convention of the other selectors? I.e. why is this not selected_idx_
| self.n_features_in_ = X.shape[1] | ||
|
|
||
| if len(y.shape) == 1: | ||
| y = y.reshape((len(y), 1)) |
There was a problem hiding this comment.
Do we want this to work for y.shape[1]!=0? If not there should be a check here
There was a problem hiding this comment.
the case y.shape = (n_samples, 1) is allowed
the case y.shape = (n_samples, >1) is checked by the check_X_y (argument multioutput is False by default)
I put the flag into the check to make it more apparent
skcosmo/sample_selection/_base.py
Outdated
| y_normal = convex_hull.equations[:, 0] | ||
|
|
||
| # get vertices_idx of the convex hull | ||
| vertices_idx = np.unique( |
skcosmo/sample_selection/_base.py
Outdated
|
|
||
| def _check_is_fitted(self, X): | ||
| check_is_fitted( | ||
| self, ["high_dim_idx_", "interpolator_high_dim_", "interpolator_y_"] |
There was a problem hiding this comment.
should also have vertices_idx_ or equivalent
| if np.any(np.isnan(interpolated_high_dim_feats)): | ||
| warnings.warn( | ||
| "There are samples in X with a low-dimensional part that is outside of the range of the convex surface. Distance will contain nans.", | ||
| UserWarning, |
|
Besides comments, I changed one more thing I would put this in a separate commit when rebasing |
see discussion #140 (comment)
* implements the class DirectionalConvexHull in the sample selection
submodule
* implements tests for the class DirectionalConvexHull.
see discussion #140 (comment)
Added the basic Generalised Convex Hull functionality, as outlined in A. Anelli, E. A. Engel, C. J. Pickard and M. Ceriotti, Physical Review Materials, 2018, to Sample Selection.