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.

Sign up for GitHub

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

doc/bettter document update and reset statepoint #444

Merged
merged 24 commits into from
Jan 23, 2021
Merged

doc/bettter document update and reset statepoint #444

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cdc39e8
grammar fix
cbkerr Dec 23, 2020
9c8ff91
explaing update_statepoint without using the word update
cbkerr Dec 23, 2020
af7b95a
grammar fix
cbkerr Dec 23, 2020
c865a37
noting that update_statepoint with overwrite=True is reset_statepoint
cbkerr Dec 23, 2020
68f3d1f
explaing reset_statetpoint without the word reset
cbkerr Dec 23, 2020
c5db8e9
add links to main signac-docs
cbkerr Dec 23, 2020
0200ada
Merge branch 'master' into doc/bettter-document-reset
cbkerr Dec 23, 2020
9309616
add noqa for long line url
cbkerr Dec 23, 2020
baf77d9
move noqa to end of docstring
cbkerr Dec 23, 2020
aee8cd3
Merge branch 'master' into doc/bettter-document-reset
bdice Dec 28, 2020
5b8fd1a
expand phrase into sentence from code review
cbkerr Dec 29, 2020
23ac203
improve wording of overwrite argument docstring
cbkerr Dec 29, 2020
661f13c
fix whitespace
cbkerr Dec 29, 2020
192db01
Merge branch 'master' into doc/bettter-document-reset
bdice Dec 31, 2020
dfe5e5d
apply documentation changes to deprecated equivalent methods
cbkerr Dec 31, 2020
daedb33
Merge branch 'master' into doc/bettter-document-reset
bdice Jan 3, 2021
de55418
Merge branch 'master' into doc/bettter-document-reset
vyasr Jan 5, 2021
1c4d1ac
suggestions from code review
cbkerr Jan 22, 2021
369ddc2
Merge branch 'master' into doc/bettter-document-reset
cbkerr Jan 22, 2021
3d97c3e
Update docstring parameters.
bdice Jan 22, 2021
cab21d6
Update docstrings.
bdice Jan 22, 2021
ad8ab00
Add missing statement.
bdice Jan 22, 2021
49c19c8
Fix all Sphinx references to errors, add missing errors to public nam…
bdice Jan 22, 2021
915eacc
Merge branch 'master' into doc/bettter-document-reset
bdice Jan 23, 2021
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
25 changes: 18 additions & 7 deletions signac/contrib/job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,12 @@ def ws(self):
return self.workspace()

def reset_statepoint(self, new_statepoint):
"""Reset the state point of this job.
"""Overwrite the state point of this job while preserving job data.

This method will change the job id if the state point has been altered.

For more information, see
`Modifying the State Point <https://docs.signac.io/en/latest/jobs.html#modifying-the-state-point>`_.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

made #452 to involve newer contributors


.. danger::

Expand All @@ -196,7 +201,7 @@ def reset_statepoint(self, new_statepoint):
new_statepoint : dict
The job's new state point.

"""
""" # noqa: E501
dst = self._project.open_job(new_statepoint)
if dst == self:
return
Expand Down Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


This method will change the job id if the state point has been altered.

For more information, see
`Modifying the State Point <https://docs.signac.io/en/latest/jobs.html#modifying-the-state-point>`_.
cbkerr marked this conversation as resolved.
Show resolved Hide resolved

.. warning::

Expand All @@ -256,9 +266,10 @@ def update_statepoint(self, update, overwrite=False):
update : dict
A mapping used for the state point update.
overwrite :
bdice marked this conversation as resolved.
Show resolved Hide resolved
Set to true, to ignore whether this update overwrites parameters,
which are currently part of the job's state point.
Use with caution! (Default value = False)
If True, this method will set all existing and new parameters
to a job's statepoint, making it equivalent to
:meth:`~.reset_statepoint`. Use with caution!
(Default value = False)

Raises
------
Expand All @@ -270,7 +281,7 @@ def update_statepoint(self, update, overwrite=False):
OSError
If the move failed due to an unknown system related error.

"""
""" # noqa: E501
statepoint = self.statepoint()
if not overwrite:
for key, value in update.items():
Expand Down
8 changes: 6 additions & 2 deletions signac/contrib/project.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1408,7 +1408,9 @@ def create_linked_view(self, prefix=None, job_ids=None, index=None, path=None):
details="Use job.reset_statepoint() instead.",
)
def reset_statepoint(self, job, new_statepoint):
"""Reset the state point of job.
"""Overwrite the state point of this job while preserving job data.

This method will change the job id if the state point has been altered.

.. danger::

Expand Down Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@bdice bdice Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


This method will change the job id if the state point has been altered.

.. warning::

Expand Down