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

update contributing guide #198

merged 55 commits into from
Aug 8, 2024

Conversation

clewis7
Copy link
Collaborator

@clewis7 clewis7 commented Jul 23, 2024

closes #196

Inspiration from plenoptic contributing guide

Major updates to the contributing guide walking users through how to contribute to nemos including:

  • development cycle
  • adding new tests
  • adding documentation
  • adding specific features (regularizer, solver method, model, basis)

@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 98.09886% with 5 lines in your changes missing coverage. Please review.

Project coverage is 97.22%. Comparing base (23433ce) to head (7430340).
Report is 130 commits behind head on development.

Files Patch % Lines
src/nemos/base_regressor.py 96.35% 5 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #198      +/-   ##
===============================================
- Coverage        97.23%   97.22%   -0.02%     
===============================================
  Files               15       18       +3     
  Lines             1484     1622     +138     
===============================================
+ Hits              1443     1577     +134     
- Misses              41       45       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clewis7
Copy link
Collaborator Author

clewis7 commented Jul 23, 2024

@BalzaniEdoardo made some progress on the contributing guide, left the section on adding a new optimizer and basis empty...I'm not as familiar with how those should look

happy to update or add anything you think is missing :D

Copy link
Collaborator

@BalzaniEdoardo BalzaniEdoardo left a comment

Choose a reason for hiding this comment

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

wow, great, I added some comments on what you did. I am going to tackle the sections that are missing

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved

```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

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
@clewis7 clewis7 changed the base branch from main to development July 24, 2024 17:59
@clewis7 clewis7 marked this pull request as ready for review July 24, 2024 20:58
@clewis7 clewis7 requested a review from billbrod July 24, 2024 20:58
Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

This looks really good Caitlin, thanks for putting in the work! I've got some notes, both here and scattered throughout.

  • Curious about the changes to the workflow files in .github, what's going on there?
  • you used **_NOTE_** in several places. I only remarked on it once or twice, but I think there is a supported admonition-like syntax that is probably more what you want here
  • Small note, and I know I'm guilty of this too (because I use emacs), but we should probably come up with guidance on line wraps in markdown files / blocks. they're inconsistent here (and I know Edoardo and I have done it differently in the past). Should we use line wraps for a single paragraph? if so, what line length? or should every paragraph be a single line?
  • You might want to describe releases: what system do we use for coming with version labels, what must be done to create a release

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/developers_notes/04-regularizer.md Show resolved Hide resolved
docs/developers_notes/05-observation_models.md Outdated Show resolved Hide resolved
docs/developers_notes/06-glm.md Show resolved Hide resolved
docs/developers_notes/06-glm.md Show resolved Hide resolved
docs/developers_notes/06-glm.md Outdated Show resolved Hide resolved
@BalzaniEdoardo
Copy link
Collaborator

This looks really good Caitlin, thanks for putting in the work! I've got some notes, both here and scattered throughout.

* Curious about the changes to the workflow files in .github, what's going on there?

This prevent triggering the ci on drafts

* you used `**_NOTE_**` in several places. I only remarked on it once or twice, but I think there is a supported [admonition-like syntax](https://github.com/orgs/community/discussions/16925) that is probably more what you want here

fixed

* Small note, and I know I'm guilty of this too (because I use emacs), but we should probably come up with guidance on line wraps in markdown files / blocks. they're inconsistent here (and I know Edoardo and I have done it differently in the past). Should we use line wraps for a single paragraph? if so, what line length? or should every paragraph be a single line?

I don't have a big opinion on that, up to you

* You might want to describe releases: what system do we use for coming with version labels, what must be done to create a release

@billbrod can you add some text on that, I think you are the only one that actually played around with "setuptools-scm"?

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated
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

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/developers_notes/03-base_regressor.md Outdated Show resolved Hide resolved
docs/developers_notes/03-base_regressor.md Show resolved Hide resolved
docs/developers_notes/03-base_regressor.md Show resolved Hide resolved
docs/developers_notes/04-regularizer.md Show resolved Hide resolved
docs/developers_notes/06-glm.md Show resolved Hide resolved
docs/developers_notes/03-base_regressor.md Show resolved Hide resolved
Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

This looks good to me! Just two small additions I'd like to see:

  • I think we should describe tox in CONTRIBUTING somewhere, even if we don't recommend people generally use it. The main reason is because tox is what happens in the test, so it's what people need to make sure passes, and we can link to the tox.ini file to show all the commands that are run (whereas this file will likely fall out of date if we change the commands)
  • For the schematic in 06-glm.md:
    • we currently have both png and svg versions of the file, but only the png is used. why are both here?
    • is this created manually or scripted? if it's scripted, I think we should just include the script here and run it during the docs build. that will be easier to keep in sync with future changes.

Note that I also pushed some changes myself, double-check whether they all look good to you.

@BalzaniEdoardo
Copy link
Collaborator

This looks good to me! Just two small additions I'd like to see:

* I think we should describe tox in CONTRIBUTING somewhere, even if we don't recommend people generally use it. The main reason is because tox is what happens in the test, so it's what people need to make sure passes, and we can link to the `tox.ini` file to show all the commands that are run (whereas this file will likely fall out of date if we change the commands)

I'll add a paragraph about this

* For the schematic in `06-glm.md`:
  
  * we currently have both png and svg versions of the file, but only the png is used. why are both here?
  * is this created manually or scripted? if it's scripted, I think we should just include the script here and run it during the docs build. that will be easier to keep in sync with future changes.

The svg is create by pyreverse but the arrows and boxes placement is not well organized, so i edited it manually. I am keeping the svg around if I'll need more manual edits. If it is not to heavy I would keep it

Note that I also pushed some changes myself, double-check whether they all look good to you.

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

Just the wording changes I proposed, and then this looks goodt o me!

CONTRIBUTING.md Outdated Show resolved Hide resolved
docs/developers_notes/04-regularizer.md Outdated Show resolved Hide resolved
BalzaniEdoardo and others added 2 commits August 8, 2024 08:50
Co-authored-by: William F. Broderick <[email protected]>
@BalzaniEdoardo BalzaniEdoardo merged commit 90d7ffc into development Aug 8, 2024
10 of 11 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the contributing_guide branch August 8, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

more extensive contributing guide
4 participants