Skip to content

Commit

Permalink
Updated Readmes
Browse files Browse the repository at this point in the history
  • Loading branch information
Esben Jannik Bjerrum committed Sep 28, 2024
1 parent 32c7d50 commit 64c0ded
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 12 deletions.
35 changes: 23 additions & 12 deletions CONTRIBUTION.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@

Thanks for your interest in contributing to the project. Please read on in the sections that apply.


## Slack channel
We have a slack channel for communication, ask for an invite: [email protected]

We have a slack channel for communication, ask for an invite: [email protected]
It's not really active and Slack wan't to be paid now. Maybe we can use Discord instead.

## Installation

Clone and install in editable more like this

git clone [email protected]:EBjerrum/scikit-mol.git
Expand All @@ -16,33 +17,40 @@ Clone and install in editable more like this
If you get issues that the editable mode install needs a setup.py, you should update your pip

## Adding transformers

The projects transformers subclasses the BaseEstimator and Transformer mixin classes from sklearn. Their documentation page contains information on what requisites are necessary [https://scikit-learn.org/stable/developers/develop.html](https://scikit-learn.org/stable/developers/develop.html). Most notably:
* The arguments accepted by __init__ should all be keyword arguments with a default value.
* Every keyword argument accepted by __init__ should correspond to an attribute on the instance.
* * There should be no logic, not even input validation, and the parameters should not be changed.
Scikit-learn classes depends on this in order to for e.g. the .get_params(), .set_params(), cloning abilities and representation rendering to work.

- The arguments accepted by **init** should all be keyword arguments with a default value.
- Every keyword argument accepted by **init** should correspond to an attribute on the instance.
- - There should be no logic, not even input validation, and the parameters should not be changed.
Scikit-learn classes depends on this in order to for e.g. the .get_params(), .set_params(), cloning abilities and representation rendering to work.

### Tips
* We have observed that some external tools used "exotic" types such at np.int64 when doing hyperparameter tuning. It is thus necessary to cast to standard types before making calls to rdkit functions. This behaviour is tested in the test_parameter_types test

* @property getters and setters can be used if additional logic are needed when setting the attributes from the keywords while at the same time adhering to the sklearn requisites.
- We have observed that some external tools used "exotic" types such at np.int64 when doing hyperparameter tuning. It is thus necessary to cast to standard types before making calls to rdkit functions. This behaviour is tested in the test_parameter_types test

* Some RDKit features uses objects as generators which may not be picklable. If instantiated and added to the object as an attribute rather than instantiated at each function call for individual molecules, these should thus be removed and recreated via overloading the _get_state() and _set_state() methods. See MHFingerprintTransformer for an example.
- @property getters and setters can be used if additional logic are needed when setting the attributes from the keywords while at the same time adhering to the sklearn requisites.

- Some RDKit features uses objects as generators which may not be picklable. If instantiated and added to the object as an attribute rather than instantiated at each function call for individual molecules, these should thus be removed and recreated via overloading the \_get_state() and \_set_state() methods. See MHFingerprintTransformer for an example.

## Module organisation

Currently we have multiple classes in the same file, if they are the same type. This may change in the future.

## Docstrings

We should ultimately consolidate on the NumPy docstring format [https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard](https://numpydoc.readthedocs.io/en/latest/format.html#docstring-standard) which is also used by SciPy and other scikits.

## Typehints

parameters and output of methods should preferably be using typehints

## Testing

New transformer classes should be added to the pytest tests in the tests directory. A lot of tests are made general, and tests aspects of the transformers that are needed for sklearn compliance or other features. The transformer is then added to a fixture and can be added to the lists of transformer objects that are run by these test. Specific tests may also be necessary to set up. As exampe the assert_transformer_set_params needs a list of non-default parameters in order to set the set_params functionality of the object.

## Notebooks

Another way of contributing is by providing notebooks with examples on how to use the project to build models together with Scikit-Learn and other tools. There is both .ipynb and #%% delimited .py files in the notebook directory as the first are useful for online rendering on github, whereas the later is useful for sub version control.

There are three scripts for handling the notebooks and their associated python:percent scripts (with much nicer diff for git).
Expand All @@ -51,9 +59,12 @@ There are three scripts for handling the notebooks and their associated python:p

`sync_notebooks.sh` uses jupytext to sync .py and ipynb. Jupytext is available via conda-forge or pip

`update_notebooks.sh` will sync, run and save the notebooks, expects a ipython kernel with scikit-mol installed called Python3.


`update_notebooks.sh` will sync, run and save the notebooks, expects a ipython kernel with scikit-mol installed called Python3.

## Release

_PyPi_
adding a new tag will trigger a release on pypi.

_Conda_
When you make a release on pypi the conda-forge bot will automatically make a PR that updates the Conda feedstock to the new version. If new dependencies or pins are changed on dependencies, those changes will need to be added to the PR. If there is just a pure code change then all we have do to is merge in the PR and that will update the package on conda-forge. See https://conda-forge.org/docs/maintainer/updating_pkgs/ for more information
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ or from conda-forge

conda install -c conda-forge scikit-mol

The conda forge package should get updated shortly after a new tagged release on pypi.

Bleeding edge

pip install git+https://github.com:EBjerrum/scikit-mol.git
Expand Down

0 comments on commit 64c0ded

Please sign in to comment.