Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update contributing guide #198

Merged
merged 55 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
265b4cc
modified connect
BalzaniEdoardo Jul 22, 2024
be6e379
start working on updating contributing guide
clewis7 Jul 23, 2024
e63ad21
more contrib guide, stop CI run on draft
clewis7 Jul 23, 2024
a5d36bf
don't think this should run on draft either
clewis7 Jul 23, 2024
12fa0b6
added description on how to contribute to basis and updated dev notes
BalzaniEdoardo Jul 24, 2024
a00536b
changeing to dev notes
BalzaniEdoardo Jul 24, 2024
e68ccfc
merged dev
BalzaniEdoardo Jul 24, 2024
5ecc8cc
requested changes
clewis7 Jul 24, 2024
a0e873b
updated base regressor note
BalzaniEdoardo Jul 24, 2024
9672994
updated note on observation models
BalzaniEdoardo Jul 24, 2024
f33b559
fixed regularizer note
BalzaniEdoardo Jul 24, 2024
8cbe550
uodated glm guidelines
BalzaniEdoardo Jul 24, 2024
ad4d5e7
removed warning
BalzaniEdoardo Jul 24, 2024
3c127b5
renamed stuff
BalzaniEdoardo Jul 24, 2024
0344360
renamed stuff
BalzaniEdoardo Jul 24, 2024
fb35931
Merge branch 'contributing_guide' of github.com:flatironinstitute/nem…
BalzaniEdoardo Jul 24, 2024
849d55a
updated descr
BalzaniEdoardo Jul 24, 2024
df263d9
add more info
BalzaniEdoardo Jul 24, 2024
05ba9bf
correct method name
BalzaniEdoardo Jul 24, 2024
680027e
some minor tweaks
clewis7 Jul 24, 2024
8004c0a
small test typo
clewis7 Jul 24, 2024
33372fd
modified scheme
BalzaniEdoardo Jul 25, 2024
26dd16b
Merge branch 'contributing_guide' of github.com:flatironinstitute/nem…
BalzaniEdoardo Jul 25, 2024
b762094
Update CONTRIBUTING.md
BalzaniEdoardo Jul 25, 2024
55f6218
Update CONTRIBUTING.md
BalzaniEdoardo Jul 25, 2024
b9fd3f0
modified notes
BalzaniEdoardo Jul 25, 2024
393b882
fixed gitflow info
BalzaniEdoardo Jul 25, 2024
ddfed69
fixed notes
BalzaniEdoardo Jul 25, 2024
15008f4
added legend
BalzaniEdoardo Jul 25, 2024
7753105
added example
BalzaniEdoardo Jul 25, 2024
07c3494
added more info on docs
BalzaniEdoardo Jul 25, 2024
da1ba91
added more info on ipynb
BalzaniEdoardo Jul 25, 2024
f29c8e1
added info on methods
BalzaniEdoardo Jul 25, 2024
1fb8f46
typo
BalzaniEdoardo Jul 25, 2024
fc27bb4
update called
BalzaniEdoardo Jul 25, 2024
91611d9
describe the attrs and how they should be used
BalzaniEdoardo Jul 25, 2024
d748877
removed examples and concrete implementations
BalzaniEdoardo Jul 25, 2024
2ee3fde
removed examples and concrete implementations
BalzaniEdoardo Jul 25, 2024
ec9b078
improve guidelines
BalzaniEdoardo Jul 25, 2024
48c33e8
important mark
BalzaniEdoardo Jul 25, 2024
5da9ab1
updated info about gamma
BalzaniEdoardo Jul 26, 2024
b94951a
updated info about Base
BalzaniEdoardo Jul 26, 2024
de71113
fixed grammar
BalzaniEdoardo Jul 26, 2024
fe98584
fixed grammar
BalzaniEdoardo Jul 26, 2024
14b6a1e
imporoved description
BalzaniEdoardo Jul 26, 2024
058a738
fix name of workflow
billbrod Aug 5, 2024
0109eb5
fix alert syntax
billbrod Aug 5, 2024
7e2be08
adds missing word
billbrod Aug 5, 2024
a6d5a4b
small text fix
billbrod Aug 5, 2024
17d1a24
describe releases
billbrod Aug 5, 2024
d3e11fe
merged
BalzaniEdoardo Aug 6, 2024
ce30a41
added note on tox
BalzaniEdoardo Aug 6, 2024
7d47df5
updated tox ini
BalzaniEdoardo Aug 6, 2024
47bed51
Update CONTRIBUTING.md
BalzaniEdoardo Aug 8, 2024
7430340
Update docs/developers_notes/04-regularizer.md
BalzaniEdoardo Aug 8, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,18 @@ on:
branches:
- main
- development
types: [opened, synchronize, reopened, ready_for_review]
types:
- opened
- reopened
- synchronize
- ready_for_review
push:
branches:
- main

jobs:
tox:
if: ${{ !github.event.pull_request.draft }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest, windows-latest]
Expand Down Expand Up @@ -45,7 +50,7 @@ jobs:
CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}

check:
if: always()
if: ${{ !github.event.pull_request.draft }}
needs: tox
runs-on: ubuntu-latest
steps:
Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/connect.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ jobs:
name: build # Name of the job
strategy:
matrix:
os: [ubuntu-latest] #[ubuntu-latest, macos-latest, windows-latest]
python-version: ['3.8', '3.9', '3.10']
os: [ubuntu-latest, macos-latest, windows-latest]
python-version: ['3.9']
runs-on: ${{ matrix.os }} # Operating system for the job
steps:
- name: Checkout # Step to checkout the repository
uses: actions/checkout@v3
uses: actions/checkout@v4

- name: Set up Python # Step to set up Python
uses: actions/setup-python@v4 # Use v4 for compatibility with pyproject.toml
Expand Down
192 changes: 190 additions & 2 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,9 +1,197 @@
# Contributing

The NeMoS package is designed to provide a robust set of statistical analysis tools for neuroscience research. While the repository is managed by a core team of data scientists at the Center for Computational Neuroscience of the Flatiron Institute, we warmly welcome contributions from external collaborators.
The NeMoS package is designed to provide a robust set of statistical analysis tools for neuroscience research. While the repository is managed by a core team of data scientists at the Center for Computational Neuroscience of the Flatiron Institute, we warmly welcome contributions from external collaborators.
This guide explains how to contribute: if you have questions about the process, please feel free to reach out on [Github Discussions](https://github.com/flatironinstitute/nemos/discussions).

## General Guidelines

Developers are encouraged to contribute to various areas of development. This could include the creation of concrete classes, such as those for new basis function types, or the addition of further checks at evaluation. Enhancements to documentation and the overall readability of the code are also greatly appreciated.

Feel free to work on any section of code that you believe you can improve. More importantly, remember to thoroughly test all your classes and functions, and to provide clear, detailed comments within your code. This not only aids others in using your library, but also facilitates future maintenance and further development.
Feel free to work on any section of code that you believe you can improve. More importantly, remember to thoroughly test all your classes and functions, and to provide clear, detailed comments within your code. This not only aids others in using the library, but also facilitates future maintenance and further development.

For more detailed information about NeMoS modules, including design choices and implementation details, visit the [`For Developers`](https://nemos.readthedocs.io/en/latest/developers_notes/) section of the package documentation.

## Contributing to the code

### Contribution workflow cycle

In order to contribute, you will need to do the following:

1) Create your own branch
BalzaniEdoardo marked this conversation as resolved.
Show resolved Hide resolved
2) Make sure that tests pass and code coverage is maintained
3) Open a Pull Request

The NeMoS package follows the [GitHub Flow](https://www.gitkraken.com/learn/git/best-practices/git-branch-strategy#github-flow-branch-strategy) workflow. In essence, this means no one is allowed to
push to the `main` branch and all development happens in separate feature branches that are then merged into `main` once we have determined they are ready. When enough changes have accumulated, we
billbrod marked this conversation as resolved.
Show resolved Hide resolved
put out a new release, adding a new tag which increments the version number, and upload the new release to PyPI.


#### Creating a development environment

You will need a local installation of `nemos` which keeps up-to-date with any changes you make. To do so, you will need to fork and clone `nemos` before checking out
a new branch:

1) Go to the [nemos repo](https://github.com/flatironinstitute/nemos) and click on the `Fork` button at the top right of the page. This will create a copy
of `nemos` in your GitHub account. You should then clone *your fork* to your local machine.

```bash
git clone https://github.com/<YourUserName>/nemos.git
cd nemos
```

2) Install `nemos` in editable mode with developer dependencies

```bash
pip install -e .[dev]
```

> [!NOTE] In order to install `nemos` in editable mode you will need a Python virtual environment. Please see our documentation [here](https://nemos.readthedocs.io/en/latest/installation/) that
> provides guidance on how to create and activate a virtual environment.

3) Add the upstream branch:

```bash
git remote add upstream https://github.com/flatironinstitute/nemos
```

At this point you have two remotes: `origin` (your fork) and `upstream` (the canonical version). You won't have permission to push to upstream (only `origin`), but
this make it easy to keep your `nemos` up-to-date with the canonical version by pulling from upstream: `git pull upstream`.

#### Creating a new branch

As mentioned previously, each feature in `nemos` is worked on in a separate branch. This allows multiple people developing multiple features simultaneously, without interfering with each other's work. To create
your own branch, run the following from within your `nemos` directory:

> **_NOTE:_** Below we are checking out the `development` branch. In terms of the `nemos` contribution workflow cycle, the `development` branch accumulates a series of
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
> **_NOTE:_** Below we are checking out the `development` branch. In terms of the `nemos` contribution workflow cycle, the `development` branch accumulates a series of
> [!NOTE] Below we are checking out the `development` branch. In terms of the `nemos` contribution workflow cycle, the `development` branch accumulates a series of

> changes from different feature branches that are then all merged into the `main` branch at one time (normally at the time of a release) :D
BalzaniEdoardo marked this conversation as resolved.
Show resolved Hide resolved

```bash
# switch to the development branch on your local copy
git checkout development
# update your local copy from your fork
git pull origin development
# sync your local copy with upstream development
git pull upstream development
# update your fork's development branch with any changs from upstream
git push origin development
# create and switch to a new branch, where you'll work on your new feature
git checkout -b my_feature_branch
```

After you have made changes on this branch, add and commit them when you are ready:

```bash
# stage the changes
git add src/nemos/the_changed_file.py
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's your opinion of using:
git add -u .

for adding any updated file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only thought is that sometimes there are files that we don't want people to commit. For example, I often test things in a juypter notebook but I would never want to commit that. I think it is better to have people do git status and then selectively choose the correct files.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are the notebook are part of the repo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's a bad idea to tell people to use git add . (though I wasn't aware of -u, that's a handy flag, and avoids many of the potential problems). but I agree with Caitlin that we want people to be selective in what they add

# commit your changes
git commit -m "A one-line message explaining the changes made"
# push to the remote origin
git push origin my_feature_branch
```

#### Contributing your change back to NeMoS

You can make any number of changes on your branch. Once you are happy with your changes, add tests to check that they run correctly and add documentation to properly note your changes.
See below for details on how to [add tests](#adding-tests) and properly [document](#adding-documentation) your code.

Lastly, you should make sure that the existing tests all run successfully and that the codebase is formatted properly:

```bash
# run tests and make sure they all pass
pytest tests/
# format the code base
black src/
clewis7 marked this conversation as resolved.
Show resolved Hide resolved
isort src
flake8 --config=tox.ini src
```
billbrod marked this conversation as resolved.
Show resolved Hide resolved

> **_INFO:_** [`black`](https://black.readthedocs.io/en/stable/) and [`isort`](https://pycqa.github.io/isort/) automatically
> reformat your code and organize your imports, respectively. [`flake8`](https://flake8.pycqa.org/en/stable/#) does not modify your
> code directly; instead, it identifies syntax errors and code complexity issues that need to be addressed manually.

> **_NOTE:_** If some files were reformatted after running `black`, make sure to commit those changes and push them to your feature branch as well.
clewis7 marked this conversation as resolved.
Show resolved Hide resolved

Now you are ready to make a Pull Request. You can open a pull request by clicking on the big `Compare & pull request` button that appears at the top of the `nemos` repo
after pushing to your branch (see [here](https://intersect-training.org/collaborative-git/03-pr/index.html) for a tutorial).

Your pull request should include the following:
- A summary including information on what you changed and why
- References to relevant issues or discussions
- Special notice to any portion of your changes where you have lingering questions (e.g., "was this the right way to implement this?") or want reviewers to pay special attention to

Next, we will be notified of the pull request and will read it over. We will try to give an initial response quickly, and then do a longer in-depth review, at which point
you will probably need to respond to our comments, making changes as appropriate. We'll then respond again, and proceed in an iterative fashion until everyone is happy with the proposed
changes.

Once your changes are integrated, you will be added as a GitHub contributor and as one of the authors of the package. Thank you for being part of `nemos`!

### Style Guide

The next section will talk about the style of your code and specific requirements for certain feature development in `nemos`.

- Longer, descriptive names are preferred (e.g., x is not an appropriate name for a variable), especially for anything user-facing, such as methods, attributes, or arguments.
- Any public method or function must have a complete type-annotated docstring (see below for details). Hidden ones do not need to have complete docstrings, but they probably should.

### Testing

To run all tests, run `pytest` from within the main `nemos` repository. This may take a while as there are many tests, broken into several categories.
There are several options for how to run a subset of tests:
- Run tests from one file: `pytest tests/test_glm.py`
- Run a specific test within a specific module: `pytests tests/test_glm.py::test_func`
- Another example specifying a test method via the command line: `pytest tests/test_glm.py::GLMClass::test_func`

#### Adding tests

New tests can be added in any of the existing `tests/test_*.py` scripts. Tests should be functions, contained within classes. The class contains a bunch of related tests
(e.g., regularizers, bases), and each test should ideally be a unit test, only testing one thing. The classes should be named `TestSomething`, while test functions should be named
`test_something` in snakecase.

If you're adding a substantial bunch of tests that are separate from the existing ones, you can create a new test script. Its name must begin with `test_`,
it must have an `.py` extension, and it must be contained within the `tests` directory. Assuming you do that, our github actions will automatically find it and
add it to the tests-to-run.

> **_NOTE:_** If you have many variants on a test you wish to run, you should make use of pytest's `parameterize` mark. See the official
> documentation [here](https://docs.pytest.org/en/stable/how-to/parametrize.html).
BalzaniEdoardo marked this conversation as resolved.
Show resolved Hide resolved

> **_NOTE_** If you are using an object that gets used in multiple tests (such as a model with certain data, regularizer, or solver), you should use pytest's `fixtures` to avoid having to
> load or instantiate the object multiple times. Look at our `conftest.py` to see already available fixtures for your tests. See the official documentation
> [here](https://docs.pytest.org/en/stable/how-to/fixtures.html).
billbrod marked this conversation as resolved.
Show resolved Hide resolved

### Documentation

Documentation is a crucial part of open-source software and greatly influences the ability to use a codebase. As such, it is imperative that any new changes are
properly documented as outlined below.

#### Adding documentation

1) **Docstrings**

All public-facing functions and classes should have complete docstrings, which start with a one-line short summary of the function,
a medium-length description of the function / class and what it does, and a complete description of all arguments and return values.
Math should be included in a `Notes` section when necessary to explain what the function is doing, and references to primary literature
should be included in a `References` section when appropriate. Docstrings should be relatively short, providing the information necessary
for a user to use the code.

Private functions and classes should have sufficient explanation that other developers know what the function / class does and how to use it,
but do not need to be as extensive.

We follow the [numpydoc](https://numpydoc.readthedocs.io/en/latest/) conventions for docstring structure.

2) **Examples/Tutorials**
billbrod marked this conversation as resolved.
Show resolved Hide resolved

If your changes are significant (add a new functionality or drastically change the current codebase), then the current examples may need to be updated or
a new example may need to be added.

All examples live within the `docs/` subfolder of `nemos`.

To see if changes you have made break the current documentation, you can build the documentation locally.

```bash
clewis7 marked this conversation as resolved.
Show resolved Hide resolved
# Clear the cached documentation pages
BalzaniEdoardo marked this conversation as resolved.
Show resolved Hide resolved
# This step is only necessary if your changes affected the src/ directory
rm -r docs/generated
# build the docs within the nemos repo
mkdocs build
```

If the build fails, you will see line-specific errors that prompted the failure.
28 changes: 18 additions & 10 deletions docs/developers_notes/01-basis_module.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,27 @@ Abstract Class Basis
└─ Concrete Subclass OrthExponentialBasis
```

The super-class `Basis` provides two public methods, [`evaluate`](#the-public-method-evaluate) and [`evaluate_on_grid`](#the-public-method-evaluate_on_grid). These methods perform checks on both the input provided by the user and the output of the evaluation to ensure correctness, and are thus considered "safe". They both make use of the private abstract method `_evaluate` that is specific for each concrete class. See below for more details.
The super-class `Basis` provides two public methods, [`compute_features`](#the-public-method-evaluate) and [`evaluate_on_grid`](#the-public-method-evaluate_on_grid). These methods perform checks on both the input provided by the user and the output of the evaluation to ensure correctness, and are thus considered "safe". They both make use of the abstract method `__call__` that is specific for each concrete class. See below for more details.

## The Class `nemos.basis.Basis`

### The Public Method `evaluate`
### The Public Method `compute_features`

The `evaluate` method checks input consistency and evaluates the basis function at some sample points. It accepts one or more numpy arrays as input, which represent the sample points at which the basis will be evaluated, and performs the following steps:
The `compute_features` method checks input consistency and applies the basis function to the inputs.
`Basis` can operate in two modes defined at initialization: `"eval"` and `"conv"`. When a basis is in mode `"eval"`,
`compute_features` evaluates the basis at the given input samples. When in mode `"conv"`, it will convolve the samples
with a bank of kernels, one per basis function.

It accepts one or more NumPy array or pynapple `Tsd` object as input, and performs the following steps:

1. Checks that the inputs all have the same sample size `M`, and raises a `ValueError` if this is not the case.
2. Checks that the number of inputs matches what the basis being evaluated expects (e.g., one input for a 1-D basis, N inputs for an N-D basis, or the sum of N 1-D bases), and raises a `ValueError` if this is not the case.
3. Calls the `_evaluate` method on the input, which is the subclass-specific implementation of the basis set evaluation.
4. Returns a numpy array of shape `(M, n_basis_funcs)`, with each basis element evaluated at the samples.
3. In `"eval"` mode, calls the `__call__` method on the input, which is the subclass-specific implementation of the basis set evaluation. In `"conv"` mode, generates a filter bank using `evaluate_on_grid` and then applies the convolution to the input with `nemos.convolve.create_convolutional_predictor`.
4. Returns a NumPy array or pynapple `TsdFrame` of shape `(M, n_basis_funcs)`, with each basis element evaluated at the samples.

!!! note "Multiple epochs"
Note that the convolution works gracefully with multiple disjoint epochs, when a pynapple time series is used as
input.

### The Public Method `evaluate_on_grid`

Expand All @@ -47,14 +56,14 @@ This method performs the following steps:

1. Checks that the number of inputs matches what the basis being evaluated expects (e.g., one input for a 1-D basis, N inputs for an N-D basis, or the sum of N 1-D bases), and raises a `ValueError` if this is not the case.
2. Calls `_get_samples` method, which returns equidistant samples over the domain of the basis function. The domain may depend on the type of basis.
3. Calls the `evaluate` method.
3. Calls the `__call__` method.
4. Returns both the sample grid points of shape `(m1, ..., mN)`, and the evaluation output at each grid point of shape `(m1, ..., mN, n_basis_funcs)`, where `mi` is the number of sample points for the i-th axis of the grid.

### Abstract Methods

The `nemos.basis.Basis` class has the following abstract methods, which every concrete subclass must implement:

1. `_evaluate`: Evaluates a basis over some specified samples.
1. `__call__`: Evaluates a basis over some specified samples.
2. `_check_n_basis_min`: Checks the minimum number of basis functions required. This requirement can be specific to the type of basis.

## Contributors Guidelines
Expand All @@ -63,8 +72,7 @@ The `nemos.basis.Basis` class has the following abstract methods, which every co
To write a usable (i.e., concrete, non-abstract) basis object, you

- **Must** inherit the abstract superclass `Basis`
- **Must** define the `_evaluate` and `_check_n_basis_min` methods with the expected input/output format, see [Code References](../../reference/nemos/basis/) for the specifics.
- **Should not** overwrite the `evaluate` and `evaluate_on_grid` methods inherited from `Basis`.
- **Must** define the `__call__` and `_check_n_basis_min` methods with the expected input/output format, see [Code References](../../reference/nemos/basis/) for the specifics.
- **Should not** overwrite the `compute_features` and `evaluate_on_grid` methods inherited from `Basis`.
- **May** inherit any number of abstract intermediate classes (e.g., `SplineBasis`).
- **May** reimplement the `_get_samples` method if your basis domain differs from `[0,1]`. However, we recommend mapping the specific basis domain to `[0,1]` whenever possible.

Loading
Loading