numpydoc style(signac/contrib/job.py) #319

ac-optimus · 2020-04-02T19:12:00Z

Description

issue #315

Motivation and Context

Types of Changes

Documentation update
Bug fix
New feature
Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

I am familiar with the Contributing Guidelines.
I agree with the terms of the Contributor Agreement.
My name is on the list of contributors.
My code follows the code style guideline of this project.
The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

I have updated the API documentation as part of the package doc-strings.
I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

Change of doc string to numpydoc in schema.py

ac-optimus · 2020-04-02T19:13:49Z

signac/contrib/job.py

        pass

    def save(self):
+        """ """
        for job in self.jobs:
            job._reset_sp()


I am not sure what class _sp_save_hook does, what should I add in its doc string?

Maybe just say "used to override default save and load behavior for synced data structures" in the class description? @vyasr @mikemhenry Any other ideas? The entire class is internal, so the comments could be more oriented towards developers instead of users.

Definitely agree that the docs should be more for developers. The goal of the _sp_save_hook is to force the job's statepoint resetting to happen whenever the statepoint is changed. Resetting a statepoint requires recomputing the hash and moving the folder, which is outside the scope of just what the synced dict does, so that's what this documentation should indicate.

thanks for the details, I have added the doc string accordingly.

codecov · 2020-04-02T19:44:06Z

Codecov Report

Merging #319 into numpy_docs will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           numpy_docs     #319   +/-   ##
===========================================
  Coverage       76.19%   76.20%           
===========================================
  Files              43       43           
  Lines            7079     7080    +1     
===========================================
+ Hits             5394     5395    +1     
  Misses           1685     1685

Impacted Files	Coverage Δ
signac/contrib/job.py	`91.20% <100.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7e35293...33884ee. Read the comment docs.

ac-optimus · 2020-04-06T20:25:38Z

@mikemhenry @tommy-waltmann i have added all of the docstrings i can. Can you please help with the rest!

mikemhenry · 2020-04-08T20:08:26Z

signac/contrib/job.py

@@ -23,14 +23,22 @@


 class _sp_save_hook(object):
+    """


Maybe something like "Force the job's statepoint to reset to whenever the statepoint is changed. Resetting a statepoint requires recomputing the hash and moving the folder"

mikemhenry · 2020-04-08T20:21:20Z

signac/contrib/job.py

-            persistent JSON file, use `job.document()` instead of `job.doc`.
-            For more information, see :attr:`Job.statepoint` or
-            :class:`~signac.JSONDict`.
-        """


We should keep this warning IMHO

I think this got deleted by mistake.

mikemhenry · 2020-04-08T20:38:07Z

signac/contrib/job.py

+        Parameters
+        ----------
+        new_sp :
+            new state point to be assign.


Suggested change

new state point to be assign.

new state point to be assigned.

ac-optimus · 2020-04-09T12:26:26Z

signac/contrib/job.py

+
+        Raises
+        ------
+


@mikemhenry @tommy-waltmann @vyasr what should I put in this?

ac-optimus · 2020-04-09T12:26:33Z

signac/contrib/job.py

+
+        Raises
+        ------
+


@mikemhenry @tommy-waltmann @vyasr what should I put in this?

signac/contrib/job.py

vyasr · 2020-04-09T14:53:43Z

@ac-optimus if you don't see any exceptions raised inside a method, you can remove the Raises section of the documentation.

… modification

ac-optimus · 2020-04-10T09:59:55Z

@ac-optimus if you don't see any exceptions raised inside a method, you can remove the Raises section of the documentation.

@vyasr yes sure, actually my comments were related to the exceptions that are raised and I am confused with what to write in their descriptions.

vyasr · 2020-04-10T13:51:41Z

Oh sorry @ac-optimus that's my fault, I didn't realize that all of these functions were actually raising exceptions. Although you're asking about specific functions, this issue raises an important question about exactly what exceptions we should document. The numpydoc guide suggests that the Raises "section should be used judiciously, i.e., only for errors that are non-obvious or have a large chance of getting raised." I interpret that as also meaning that we should not document exceptions that we are not raising ourselves, i.e. don't add Exceptions to the Raises section if they're being reraised after catching them. So any function where we do something like except Exception as e:... raise e, I would just skip those (which I think is most of the ones you asked about). Does that make sense?

ac-optimus · 2020-04-10T18:21:14Z

Oh sorry @ac-optimus that's my fault, I didn't realize that all of these functions were actually raising exceptions. Although you're asking about specific functions, this issue raises an important question about exactly what exceptions we should document. The numpydoc guide suggests that the Raises "section should be used judiciously, i.e., only for errors that are non-obvious or have a large chance of getting raised." I interpret that as also meaning that we should not document exceptions that we are not raising ourselves, i.e. don't add Exceptions to the Raises section if they're being reraised after catching them. So any function where we do something like except Exception as e:... raise e, I would just skip those (which I think is most of the ones you asked about). Does that make sense?

Yes it does @vyasr . So according to that I should remove all the Raises docstrings that I have added other than the below ones?

def update_statepoint(self, update, overwrite=False):
    ...
    ...   
    ...
        Raises
        ------
        KeyError 
            If the update contains keys, which are already part of the job's
            state point and overwrite is False.

and

def sync(self, other, strategy=None, exclude=None, doc_sync=None, **kwargs):
    ...
    ...   
    ...
        Raises
        ------
        FileSyncConflict
            In case that a file synchronization results in a conflict.

mikemhenry

LGTM!

bdice · 2020-04-18T20:36:34Z

I took a quick glance at this PR. It seems like some docstrings might be indicating the wrong behavior or have the wrong types (e.g. iterables of jobs vs. a single job). Also I saw some rST errors with .. warning:: that will prevent the docs from rendering properly, as well as spelling/grammar errors. I am working on a few other documentation pull requests like #318, #320, and #322. @mikemhenry could you take another look? Or maybe @vyasr since you've commented on this PR as well?

vyasr

I have a number of changes, but you should be able to just batch commit all my suggestions and then I think this is good. The only thing I'm somewhat uncertain about is how things like referring to Job will work with numpydoc. I've looked online for examples because numpydoc's documentation doesn't say anything about it, and it seems like it should properly link with doing the :class:-type domain prefix, but I don't know if there are any limitations to this. @ac-optimus it would be great if you could try and build this locally once to just see if the links work so that we know for future PRs.

signac/contrib/job.py

ac-optimus · 2020-04-21T18:34:49Z

I have a number of changes, but you should be able to just batch commit all my suggestions and then I think this is good. The only thing I'm somewhat uncertain about is how things like referring to Job will work with numpydoc. I've looked online for examples because numpydoc's documentation doesn't say anything about it, and it seems like it should properly link with doing the :class:-type domain prefix, but I don't know if there are any limitations to this. @ac-optimus it would be great if you could try and build this locally once to just see if the links work so that we know for future PRs.

sure, working on it...

tommy-waltmann

Address the comments from @vyasr and I think we'll be good to go

ac-optimus · 2020-04-23T07:41:42Z

@tommy-waltmann I think I have addressed them all. Any specific one, just in case I missed it?

tommy-waltmann · 2020-04-23T13:21:35Z

@tommy-waltmann I think I have addressed them all. Any specific one, just in case I missed it?

I was referring to trying to build the documentation locally to see if numpydoc works as we expect. Otherwise, I think this PR is in good shape.

ac-optimus · 2020-04-23T14:24:41Z

@tommy-waltmann I think I have addressed them all. Any specific one, just in case I missed it?

I was referring to trying to build the documentation locally to see if numpydoc works as we expect. Otherwise, I think this PR is in good shape.

Yes, I tried it and even had conversation with @vyasr as well on slack. This is working fine!

* numpydoc style(signac/contrib/schema.py) (#318) * numpydoc style(signac/contrib/project.py) (#320) * numpydoc style(signac/contrib/job.py) (#319) * numpydoc style(signac/contrib/linked_view.py, signac/contrib/utility.py, signac/core/utility.py) (#326) * numpydoc style(signac/contrib/errors.py, signac/contrib/filesystems.py, signac/contrib/filterparser.py, signac/contrib/hashing.py) (#322) * numpydoc style (signac/contrib/collection.py) (#329) * Make core submodule adhere to numpydoc conventions. (#339) * add a docstring for public module (#337) * numpydoc style (signac/contrib/import_export.py) (#333) * Apply NumPy docstyle to remaining files in "signac" root. (#343) * Apply fixes from minirst. (#356) * Enforce pydocstyle in core, common, contrib. (#354) Co-authored-by: Bradley Dice <[email protected]> Co-authored-by: Alyssa Travitz <[email protected]> Co-authored-by: Abhavya Chandra <[email protected]> Co-authored-by: vyasr <[email protected]> Co-authored-by: Pengji <[email protected]> Co-authored-by: Mike Henry <[email protected]>

ac-optimus added 2 commits April 3, 2020 00:30

numpydoc stype signac/contrib/job.py

b92e3f1

Merge branch 'master' into job_numpydoc

98310cb

ac-optimus requested review from a team as code owners April 2, 2020 19:12

ac-optimus commented Apr 2, 2020

View reviewed changes

vyasr requested review from a team, tommy-waltmann and mikemhenry and removed request for a team April 4, 2020 16:53

doc-string numpydoc style change

ab52592

mikemhenry requested changes Apr 8, 2020

View reviewed changes

ac-optimus added 3 commits April 9, 2020 17:18

making suggested changes

461f876

chagnes to docstring.

8694c5c

adding warning.

c2f3162

ac-optimus commented Apr 9, 2020

View reviewed changes

signac/contrib/job.py Outdated

Raises

------

Copy link

Contributor Author

ac-optimus Apr 9, 2020

Choose a reason for hiding this comment

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

@mikemhenry @tommy-waltmann @vyasr what should I put in this?

ac-optimus commented Apr 9, 2020

View reviewed changes

signac/contrib/job.py Outdated

Raises

------

Copy link

Contributor Author

ac-optimus Apr 9, 2020

Choose a reason for hiding this comment

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

@mikemhenry @tommy-waltmann @vyasr what should I put in this?

ac-optimus commented Apr 9, 2020

View reviewed changes

signac/contrib/job.py Show resolved Hide resolved

adding contrib/jb.py in setup.cfg + removing some errors in doc style…

36907ba

… modification

ac-optimus requested review from mikemhenry and vyasr April 10, 2020 11:22

vyasr removed their request for review April 10, 2020 13:52

ac-optimus added 2 commits April 16, 2020 14:33

making changes to docstrings

540b68b

removing unwanted raises

594281a

ac-optimus requested a review from vyasr April 16, 2020 17:32

mikemhenry approved these changes Apr 17, 2020

View reviewed changes

making final(maybe) changes to docstring

58f4ca2

vyasr requested changes Apr 21, 2020

View reviewed changes

ac-optimus and others added 3 commits April 22, 2020 21:09

making changes suggested by @vyasr

07b46cc

linking classes, methods, attr, etc for better docstring render.

e7ad8c8

Merge branch 'numpy_docs' into job_numpydoc

33884ee

tommy-waltmann approved these changes Apr 22, 2020

View reviewed changes

vyasr merged commit ed69d02 into glotzerlab:numpy_docs Apr 23, 2020

bdice assigned ac-optimus Apr 24, 2020

bdice added this to the v1.5.0 milestone Apr 24, 2020

bdice added the doc-style Update docs to numpy doc style label May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

numpydoc style(signac/contrib/job.py) #319

numpydoc style(signac/contrib/job.py) #319

ac-optimus commented Apr 2, 2020

ac-optimus Apr 2, 2020

tommy-waltmann Apr 7, 2020 •

edited

Loading

vyasr Apr 8, 2020

ac-optimus Apr 9, 2020

codecov bot commented Apr 2, 2020 •

edited

Loading

ac-optimus commented Apr 6, 2020

mikemhenry Apr 8, 2020

mikemhenry Apr 8, 2020

ac-optimus Apr 9, 2020

mikemhenry Apr 8, 2020

ac-optimus Apr 9, 2020

ac-optimus Apr 9, 2020

vyasr commented Apr 9, 2020

ac-optimus commented Apr 10, 2020

vyasr commented Apr 10, 2020

ac-optimus commented Apr 10, 2020

mikemhenry left a comment

bdice commented Apr 18, 2020

vyasr left a comment

ac-optimus commented Apr 21, 2020

tommy-waltmann left a comment

ac-optimus commented Apr 23, 2020

tommy-waltmann commented Apr 23, 2020

ac-optimus commented Apr 23, 2020 •

edited

Loading

	new state point to be assign.
	new state point to be assigned.

numpydoc style(signac/contrib/job.py) #319

numpydoc style(signac/contrib/job.py) #319

Conversation

ac-optimus commented Apr 2, 2020

Description

Motivation and Context

Types of Changes

Checklist:

Choose a reason for hiding this comment

tommy-waltmann Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 2, 2020 • edited Loading

Codecov Report

ac-optimus commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr commented Apr 9, 2020

ac-optimus commented Apr 10, 2020

vyasr commented Apr 10, 2020

ac-optimus commented Apr 10, 2020

mikemhenry left a comment

Choose a reason for hiding this comment

bdice commented Apr 18, 2020

vyasr left a comment

Choose a reason for hiding this comment

ac-optimus commented Apr 21, 2020

tommy-waltmann left a comment

Choose a reason for hiding this comment

ac-optimus commented Apr 23, 2020

tommy-waltmann commented Apr 23, 2020

ac-optimus commented Apr 23, 2020 • edited Loading

tommy-waltmann Apr 7, 2020 •

edited

Loading

codecov bot commented Apr 2, 2020 •

edited

Loading

ac-optimus commented Apr 23, 2020 •

edited

Loading