search

scikit-learn-contrib / scikit-matter

A collection of scikit-learn compatible utilities that implement methods born out of the materials science and chemistry communities
https://scikit-matter.readthedocs.io/en/v0.2.0/
BSD 3-Clause "New" or "Revised" License
70 stars 18 forks source link

Added GCH functionality. #140

Closed victorprincipe closed 1 year ago

victorprincipe commented 1 year ago

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.

agoscinski commented 1 year ago

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

2022-12-08T10:58:51.8211666Z packaging.requirements.InvalidRequirement: Expected version after operator
2022-12-08T10:58:51.8212053Z     scikit-learn (>="0.24.0")
agoscinski commented 1 year ago

rebased. I have made a backup and sent to @victorprincipe in case I screwed something up

rosecers commented 1 year ago

Weren't there tests in here as well? Please make sure that there is proper code coverage.

agoscinski commented 1 year ago

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.

agoscinski commented 1 year ago

@rosecers okay seems to work now

agoscinski commented 1 year ago

We would squash everything to one commit

    Implementation of DirectionalConvexHull

    * implements the class DirectionalConvexHull in the sample selection
    submodule

    * implements tests for the class DirectionalConvexHull
agoscinski commented 1 year ago

Besides comments, I changed one more thing

    change minimum scikit-learn version to 1.1

    see discussion https://github.com/lab-cosmo/scikit-cosmo/pull/140#discussion_r1042238012

I would put this in a separate commit when rebasing