graspologic Draft PR review guidelines

Peer review checklist

PR itself (on GitHub)

[ ] PR has a descriptive and succinct title.
[ ] PR has a brief description of what was changed.
[ ] PR is addressing a change that has already been brought up in an issue. This will ensure that the change you propose is desired by the maintainers, and may provide a chance to discuss implementation details prior to PRing which will save everyone time.
[ ] PR cites any related issues in the PR description and uses closing keywords for any issues that should be closed as a result of the PR.
[ ] PR does not have any extraneous file changes associated with it. One common example of an extraneous change is one that was already made in the target branch, but is showing up as part of this PR under Files changed.
[ ] PR does not have any unresolved merge conflicts.
[ ] If implementing a major new feature or algorithm, a notebook demonstrating the proof of effectiveness is linked in the PR description. Depending on the feature, this kind of validation may take too long to run as a test or in a tutorial notebook.

Style

[ ] Code follows Python variable naming conventions, one description is here. Most variables should be snake_case and classes shoud be defined as UpperCamelCase, for example.
[ ] Code uses descriptive variable names throughout.
[ ] Code has no commented out "junk code."
[ ] Line lengths are short (recommended <88 chars). This includes docstrings. Note: black tries to shorten line lengths but sometimes it is unable to do so automatically, especially for docstrings.
[ ] New files have the appropriate header (example).

Documentation

[ ] Any new public classes/functions have docstrings.
[ ] Any new public classes/functions have been added to the appropriate .rst file here to be rendered by Sphinx.
[ ] Any major new features (like a new algorithm) are accompanied by a succinct tutorial notebook. Or, if it is agreed upon with the maintainers that this is out of scope for the current PR, a new issue has been created specifying that we need a tutorial notebook for this functionality.
[ ] Any new tutorial notebooks have been added to the appropriate folder here and included in the .rst file here to be rendered by Sphinx.
[ ] Notebooks are cleared, that is, they do not have any run cell output. This is to allow Sphinx to run the notebooks every time the documentation is built which ensures nothing has broken.
[ ] Modifications to the documentation render appropriately in the Netlify build.
[ ] References to parameters in the documentation are wrapped in double backticks (`` on either side of the parameter).
[ ] References to classes/function/documentation outside of this package use intersphinx to automatically link to the relevant documentation pages.

Testing

[ ] New public classes/functions are tested to ensure they achieve the desired output.
[ ] New public classes/functions are tested to ensure proper errors are thrown when invalid inputs are passed.

Oct 05 '20 20:10 bdpedigo

The above is my draft of what I'd like every peer reviewer in NDD to check for. The PR peer reviewer will check off each item (or request changes until each box can be checked off). After everything is checked off, the author of the PR will tag me (or the appropriate person) for review. These are all things I've had to remind people of in the past so I am asking peer reviewers to check for them first.

As we discussed in a meeting today, much of this may be worth putting in the official graspologic PR checklist as well.

Oct 05 '20 20:10 bdpedigo

https://github.com/stilliard/github-task-list-completed could be useful for looking at this at a glance, maybe as an optional check?

Oct 05 '20 20:10 bdpedigo

@dwaynepryce @j1c @asaadeldin11 @alyakin314 any thoughts on the above? I'd like to finalize this (at least for the purposes of NDD) by the end of the week so LMK.

Oct 05 '20 20:10 bdpedigo

Line lengths are short (recommended <88 chars). This includes docstrings. Note: black tries to shorten line lengths but sometimes it is unable to do so automatically, especially for docstrings.

i've meant to ask this for a while: is there a reason why aren't we using 79 chars, which seems to be the standard in PEP8 and many editors.

Oct 05 '20 20:10 alyakin314

For the PR itself portion, maybe mention that new features should include proof of effectiveness, like a jupyter notebook that replicates a figure from a paper

Oct 05 '20 20:10 asaadeldin11

For the PR itself portion, maybe mention that new features should include proof of effectiveness, like a jupyter notebook that replicates a figure from a paper

already here: https://github.com/microsoft/graspologic/blob/dev/CONTRIBUTING.md (and should eventually be in pr template; see: https://github.com/microsoft/graspologic/pull/493 )

Oct 05 '20 21:10 alyakin314

@dwaynepryce @j1c @asaadeldin11 @alyakin314 any thoughts on the above? I'd like to finalize this (at least for the purposes of NDD) by the end of the week so LMK.

rest seems good.

Oct 05 '20 21:10 alyakin314

i've meant to ask this for a while: is there a reason why aren't we using 79 chars, which seems to be the standard in PEP8 and many editors.

https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

Oct 05 '20 21:10 bdpedigo

For the PR itself portion, maybe mention that new features should include proof of effectiveness, like a jupyter notebook that replicates a figure from a paper

I can specify that the proof should be linked in the PR description, cause this kind of thing is often separate from the tests and tutorial notebook

Oct 05 '20 21:10 bdpedigo

For the PR itself portion, maybe mention that new features should include proof of effectiveness, like a jupyter notebook that replicates a figure from a paper

"If implementing a major new feature or algorithm, a notebook demonstrating the proof of effectiveness is linked in the PR description. Depending on the feature, this kind of validation may take too long to run as a test or in a tutorial notebook."

Addressed above, ok?

Oct 05 '20 21:10 bdpedigo

Also added this

"Any major new features (like a new algorithm) are accompanied by a succinct tutorial notebook. Or, if it is agreed upon with the maintainers that this is out of scope for the current PR, a new issue has been created specifying that we need a tutorial notebook for this functionality."

Oct 05 '20 21:10 bdpedigo

When do we want to introduce type hinting into the mix? After we've done the big "type hint all the things" PR and have a ton of examples to point to?

I only ask because we're just adding more work for us later if we don't do it now, but I also don't want to just say "type hint stuff lol good luck" either

Oct 05 '20 21:10 daxpryce

After we've done the big "type hint all the things" PR and have a ton of examples to point to?

This seems the most fair thing to me, personally.

Oct 05 '20 21:10 bdpedigo

I only ask because we're just adding more work for us later if we don't do it now.

Off the top of my head, I think NDD PRs (for this semester) will add a total of

1 new module consisting of 2 classes/functions
2 new functions for plotting
2 functions which replace 2 classes with the same functionality

If we want to make sure those are all type-hinted, that is also okay with me. But I do think we should have examples if so.

Oct 05 '20 21:10 bdpedigo

i've meant to ask this for a while: is there a reason why aren't we using 79 chars, which seems to be the standard in PEP8 and many editors.

https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

i can certainly believe that it

This number was found to produce significantly shorter files than sticking with 80 (the most popular), or even 79 (used by the standard library).

but personally over the time i have been contributing to graspy/graspologic it felt much more like a nuisance more than an upside because the defaults on everything else from all editors i've used since to mac terminal windows are 79/80. furthermore, being a standard in almost every other codebase, it also forces to toggle between settings when switching from graspologic to another project. by no means saying that conventions need to be followed always, but again, im(very)ho this has been more of an annoyance than upside.

Oct 05 '20 21:10 alyakin314

it also forces to toggle between settings when switching from graspologic to another project.

VS Code (and I assume any reasonable code editor) lets you apply settings per folder or per project, so I don't feel too bad about this one. Also not sure what issue you are even talking about, just Flake8 warnings or something?

IMO the advantage of everyone being able to do pip install black and not make any setting changes and instantly be on the same page is a greater benefit. I think this is the benefit of the "uncompromising" aspect of black. If you disagree further, let's discuss in a separate issue, because that'd be a package-wide change.

Oct 05 '20 21:10 bdpedigo

FWIW, I hate 80 so much we went with 120 on topologic https://github.com/microsoft/topologic/blob/dev/setup.cfg#L2

Edit: that said, this isn't "let's do what dwayne wants"-pologic, either.

Oct 05 '20 22:10 daxpryce

can you please make this a blog post so that we can reference it? or a paper for gigascience or something?

Oct 06 '20 00:10 jovo

Edit: that said, this isn't "let's do what dwayne wants"-pologic, either.

Sounds fun tbh

Oct 06 '20 00:10 bdpedigo

can you please make this a blog post so that we can reference it? or a paper for gigascience or something?

sure, for now i can draft something for the blog at least

Oct 06 '20 00:10 bdpedigo