doc/bettter document update and reset statepoint #444

cbkerr · 2020-12-23T17:39:12Z

Description

The docstrings for job.update_statepoint() and job.reset_statepoint() are confusing because they are too short and use the same words in the definition.

Here, I propose an update for them to be able to stand on their own.

I also linked them to the relevant part of signac docs (no intersphinx because the other link in the file didn't use it.)

Note: preview the changes live here: https://signac--444.org.readthedocs.build/projects/core/en/444/api.html#the-job-class

Motivation and Context

See a bigger write-up about the confusion here: glotzerlab/signac-docs#108

In an offline discussion with @vyasr last week, we also realized that the main documentation states that these functions change the job id while the docstrings do not. Here, I also propose we add that notice to the docstrings.

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.

codecov · 2020-12-23T17:58:26Z

Codecov Report

Merging #444 (49c19c8) into master (8c7362a) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #444   +/-   ##
=======================================
  Coverage   77.18%   77.18%           
=======================================
  Files          42       42           
  Lines        5768     5768           
  Branches     1129     1129           
=======================================
  Hits         4452     4452           
  Misses       1030     1030           
  Partials      286      286

Impacted Files	Coverage Δ
signac/contrib/collection.py	`84.02% <ø> (ø)`
signac/contrib/import_export.py	`77.89% <ø> (ø)`
signac/contrib/job.py	`89.12% <ø> (ø)`
signac/contrib/project.py	`86.97% <ø> (ø)`
signac/sync.py	`82.32% <ø> (ø)`

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 ed99e25...915eacc. Read the comment docs.

bdice

Thanks for the PR @cbkerr! I agree these docs can be improved. I have a few suggestions for better clarity and usage of Sphinx features.

signac/contrib/job.py

bdice · 2020-12-28T18:04:46Z

signac/contrib/job.py

+
+        Change the job id if necessary.
+        For more, see
+        `Modifying the State Point <https://docs.signac.io/en/latest/jobs.html#modifying-the-state-point>`_.


This should use an intersphinx reference to _project-job-statepoint-modify.

See this guide: https://github.com/glotzerlab/signac-docs/wiki/How-to-cross-reference-links-between-package-docs-and-signac-docs-using-intersphinx

I'll make a new issue to add intersphinx because none of the files use it yet.

made #452 to involve newer contributors

signac/contrib/job.py

Co-authored-by: Bradley Dice <[email protected]>

vyasr · 2020-12-29T22:15:17Z

@cbkerr feel free to ping me if you would like me to weigh in on any discussions, but I'll refrain from commenting too much otherwise since I think you've summarized (and expanded upon!) our discussion very nicely in glotzerlab/signac-docs#108 and here.

bdice

This is a good improvement. We need to make the same changes for Project.update_statepoint and Project.reset_statepoint. Those methods are deprecated but documentation improvements should still be applied. I would have preferred to start using intersphinx references in this PR rather than deferring that to #452, to set a good example for the future, but I understand if you wish to skip that for now.

cbkerr · 2021-01-05T17:56:42Z

not ready to merge yet!

cbkerr

figuring out best way to address @Charlottez112's comment. One option is to add to the first sentence which is good for completeness.

Another could be adding a wholely new sentence.

cbkerr · 2021-01-05T17:59:15Z

signac/contrib/project.py

@@ -1440,7 +1442,9 @@ def reset_statepoint(self, job, new_statepoint):
        details="Use job.update_statepoint() instead.",
    )
    def update_statepoint(self, job, update, overwrite=False):
-        """Update the state point of this job.
+        """Change the state point of this job without affecting existing parameters.


Suggested change

"""Change the state point of this job without affecting existing parameters.

"""Change the state point of this job without affecting existing parameters while preserving job data.

There's another small issue here, which is that "without affecting existing parameters" is only true if overwrite=False, so that might be misleading.

I agree with @vyasr that this suggestion is potentially misleading. The summary line of a docstring must be short. It doesn't have to fully explain the behavior or how various parameters affect its behavior - that's better to put in the description after the first line and the "Parameters" sections.

I made a separate sentence to address @Vyas's point. I left in "while preserving job data" in both to resolve my confusion and @Charlottez112 's suggestion

cbkerr · 2021-01-05T17:59:34Z

signac/contrib/job.py

@@ -243,7 +248,12 @@ def _reset_sp(self, new_statepoint=None):
        self.reset_statepoint(new_statepoint)

    def update_statepoint(self, update, overwrite=False):
-        """Update the state point of this job.
+        """Change the state point of this job without affecting existing parameters.


Suggested change

"""Change the state point of this job without affecting existing parameters.

"""Change the state point of this job without affecting existing parameters while preserving job data.

vyasr

This PR is waiting on @cbkerr to make some additional wording changes.

vyasr

This PR is waiting on @cbkerr to make some wording changes.

cbkerr · 2021-01-22T22:11:10Z

Some error from when I merged in the master branch on the windows build. Maybe from the recent CI changes?

WARNING: Generic MSI Error. This is a local environment error, not an issue with a package or the MSI itself - it could mean a pending reboot is necessary prior to install or something else (like the same version is already installed). Please see MSI log if available. If not, try again adding '--install-arguments="'/l*v c:\python3_msi_install.log'"'. Then search the MSI Log for "Return Value 3" and look above that for the error.
ERROR: Running ["C:\ProgramData\chocolatey\lib\python3\tools\python-3.8.1-amd64.exe" /quiet InstallAllUsers=1 PrependPath=1 TargetDir="C:\Python38" ] was not successful. Exit code was '1603'. Exit code indicates the following: Generic MSI Error. This is a local environment error, not an issue with a package or the MSI itself - it could mean a pending reboot is necessary prior to install or something else (like the same version is already installed). Please see MSI log if available. If not, try again adding '--install-arguments="'/l*v c:\python3_msi_install.log'"'. Then search the MSI Log for "Return Value 3" and look above that for the error..
  python3 may be able to be automatically uninstalled.
The install of python3 was NOT successful.
Error while running 'C:\ProgramData\chocolatey\lib\python3\tools\chocolateyInstall.ps1'.
 See log for details.```

bdice · 2021-01-22T22:28:16Z

Some error from when I merged in the master branch on the windows build. Maybe from the recent CI changes?

WARNING: Generic MSI Error. This is a local environment error, not an issue with a package or the MSI itself - it could mean a pending reboot is necessary prior to install or something else (like the same version is already installed). Please see MSI log if available. If not, try again adding '--install-arguments="'/l*v c:\python3_msi_install.log'"'. Then search the MSI Log for "Return Value 3" and look above that for the error.
ERROR: Running ["C:\ProgramData\chocolatey\lib\python3\tools\python-3.8.1-amd64.exe" /quiet InstallAllUsers=1 PrependPath=1 TargetDir="C:\Python38" ] was not successful. Exit code was '1603'. Exit code indicates the following: Generic MSI Error. This is a local environment error, not an issue with a package or the MSI itself - it could mean a pending reboot is necessary prior to install or something else (like the same version is already installed). Please see MSI log if available. If not, try again adding '--install-arguments="'/l*v c:\python3_msi_install.log'"'. Then search the MSI Log for "Return Value 3" and look above that for the error..
  python3 may be able to be automatically uninstalled.
The install of python3 was NOT successful.
Error while running 'C:\ProgramData\chocolatey\lib\python3\tools\chocolateyInstall.ps1'.
 See log for details.```

Ignore this, it's a random failure. It's been happening a fair amount in the last couple days for no apparent reason.

signac/contrib/job.py

…espace.

bdice · 2021-01-22T22:48:42Z

@cbkerr I fixed up some places where project.py and job.py didn't have the same docstring edits. You replied to a comment on one file, but made the changes in the other file (and not the place you commented). I also fixed all broken Sphinx references to error classes. I'll merge this once tests pass.

cbkerr · 2021-01-23T06:57:01Z

I think my problem was that I made the changes before merging and didn't check when the merge just worked...

Outdated

cbkerr added 7 commits December 23, 2020 12:05

grammar fix

cdc39e8

explaing update_statepoint without using the word update

9c8ff91

grammar fix

af7b95a

noting that update_statepoint with overwrite=True is reset_statepoint

c865a37

explaing reset_statetpoint without the word reset

68f3d1f

add links to main signac-docs

c5db8e9

Merge branch 'master' into doc/bettter-document-reset

0200ada

cbkerr requested a review from vyasr December 23, 2020 17:39

cbkerr requested review from a team as code owners December 23, 2020 17:39

cbkerr requested review from Charlottez112 and removed request for a team December 23, 2020 17:39

cbkerr mentioned this pull request Dec 23, 2020

Imprecise use of "reset" throughout the docs glotzerlab/signac-docs#108

Open

4 tasks

add noqa for long line url

9309616

cbkerr and others added 2 commits December 23, 2020 12:58

move noqa to end of docstring

baf77d9

Merge branch 'master' into doc/bettter-document-reset

aee8cd3

bdice requested changes Dec 28, 2020

View reviewed changes

cbkerr and others added 3 commits December 29, 2020 15:46

expand phrase into sentence from code review

5b8fd1a

Co-authored-by: Bradley Dice <[email protected]>

improve wording of overwrite argument docstring

23ac203

fix whitespace

661f13c

cbkerr mentioned this pull request Dec 29, 2020

Use intersphinx in documentation #452

Closed

cbkerr requested a review from bdice December 30, 2020 19:15

Merge branch 'master' into doc/bettter-document-reset

192db01

bdice requested changes Dec 31, 2020

View reviewed changes

apply documentation changes to deprecated equivalent methods

dfe5e5d

bdice self-requested a review December 31, 2020 19:51

bdice approved these changes Dec 31, 2020

View reviewed changes

Merge branch 'master' into doc/bettter-document-reset

daedb33

Merge branch 'master' into doc/bettter-document-reset

de55418

cbkerr commented Jan 5, 2021

View reviewed changes

vyasr approved these changes Jan 12, 2021

View reviewed changes

vyasr self-requested a review January 12, 2021 22:45

vyasr previously requested changes Jan 12, 2021

View reviewed changes

bdice assigned cbkerr Jan 22, 2021

bdice added this to the v1.6.0 milestone Jan 22, 2021

bdice added the documentation Writing or editing documentation label Jan 22, 2021

cbkerr added 2 commits January 22, 2021 16:30

suggestions from code review

1c4d1ac

Merge branch 'master' into doc/bettter-document-reset

369ddc2

cbkerr requested a review from bdice January 22, 2021 22:09

bdice reviewed Jan 22, 2021

View reviewed changes

signac/contrib/job.py Outdated Show resolved Hide resolved

bdice added 4 commits January 22, 2021 16:29

Update docstring parameters.

3d97c3e

Update docstrings.

cab21d6

Add missing statement.

ad8ab00

Fix all Sphinx references to errors, add missing errors to public nam…

49c19c8

…espace.

bdice enabled auto-merge (squash) January 22, 2021 22:48

bdice disabled auto-merge January 23, 2021 21:17

Merge branch 'master' into doc/bettter-document-reset

915eacc

bdice enabled auto-merge (squash) January 23, 2021 21:17

bdice merged commit ec58809 into master Jan 23, 2021

bdice deleted the doc/bettter-document-reset branch January 23, 2021 21:26

bdice mentioned this pull request Jan 23, 2021

Feature/synced collection/replace jsondict #472

Merged

12 tasks

bdice mentioned this pull request Feb 17, 2021

Incorrect docstring in Job.update_statepoint #506

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc/bettter document update and reset statepoint #444

doc/bettter document update and reset statepoint #444

cbkerr commented Dec 23, 2020 •

edited

Loading

codecov bot commented Dec 23, 2020 •

edited

Loading

bdice left a comment

bdice Dec 28, 2020

cbkerr Dec 29, 2020

cbkerr Dec 29, 2020

vyasr commented Dec 29, 2020

bdice left a comment •

edited

Loading

cbkerr commented Jan 5, 2021

cbkerr left a comment •

edited

Loading

cbkerr Jan 5, 2021

vyasr Jan 5, 2021

bdice Jan 5, 2021 •

edited

Loading

cbkerr Jan 22, 2021

cbkerr Jan 5, 2021

vyasr left a comment

vyasr left a comment

cbkerr commented Jan 22, 2021

bdice commented Jan 22, 2021

bdice commented Jan 22, 2021

cbkerr commented Jan 23, 2021

	"""Change the state point of this job without affecting existing parameters.
	"""Change the state point of this job without affecting existing parameters while preserving job data.

doc/bettter document update and reset statepoint #444

doc/bettter document update and reset statepoint #444

Conversation

cbkerr commented Dec 23, 2020 • edited Loading

Description

Motivation and Context

Types of Changes

Checklist:

codecov bot commented Dec 23, 2020 • edited Loading

Codecov Report

bdice left a comment

Choose a reason for hiding this comment

bdice Dec 28, 2020

Choose a reason for hiding this comment

cbkerr Dec 29, 2020

Choose a reason for hiding this comment

cbkerr Dec 29, 2020

Choose a reason for hiding this comment

vyasr commented Dec 29, 2020

bdice left a comment • edited Loading

Choose a reason for hiding this comment

cbkerr commented Jan 5, 2021

cbkerr left a comment • edited Loading

Choose a reason for hiding this comment

cbkerr Jan 5, 2021

Choose a reason for hiding this comment

vyasr Jan 5, 2021

Choose a reason for hiding this comment

bdice Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

cbkerr Jan 22, 2021

Choose a reason for hiding this comment

cbkerr Jan 5, 2021

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

cbkerr commented Jan 22, 2021

bdice commented Jan 22, 2021

bdice commented Jan 22, 2021

cbkerr commented Jan 23, 2021

cbkerr commented Dec 23, 2020 •

edited

Loading

codecov bot commented Dec 23, 2020 •

edited

Loading

bdice left a comment •

edited

Loading

cbkerr left a comment •

edited

Loading

bdice Jan 5, 2021 •

edited

Loading