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

Incorrect docstring in Job.update_statepoint #506

Closed
bdice opened this issue Feb 17, 2021 · 3 comments · Fixed by #563
Closed

Incorrect docstring in Job.update_statepoint #506

bdice opened this issue Feb 17, 2021 · 3 comments · Fixed by #563
Labels
bug Something isn't working documentation Writing or editing documentation good first issue Good for newcomers
Milestone

Comments

@bdice
Copy link
Member

bdice commented Feb 17, 2021

The docstring modified by @cbkerr in #444 has a small error / confusing wording. Here, the Job.update_statepoint method claims that setting overwrite=True makes the method update_statepoint act like the method reset_statepoint. This is not quite true, and nearly caused me to re-implement the behavior incorrectly while refactoring #497.

to a job's statepoint, making it equivalent to
:meth:`~.reset_statepoint`. Use with caution!

For example, see this minimal reproduction:

import signac
project = signac.init_project("UpdateStatepointDocstringBug")
job = project.open_job({"a": 1})
print(job.sp)
job.update_statepoint({"b": 2}, overwrite=True)   # Here the overwrite behavior doesn't matter because the key does not exist yet
print(job.sp)
job.update_statepoint({"b": 3})
print(job.sp)

Output:

{'a': 1}
{'a': 1, 'b': 2}
Traceback (most recent call last):
  File "problem.py", line 7, in <module>
    job.update_statepoint({"b": 3})
  File "/home/bdice/code/signac/signac/contrib/job.py", line 414, in update_statepoint
    raise KeyError(key)
KeyError: 'b'

Notice that when overwrite=True, the Job.update_statepoint method acts like dict.update in its behavior of overwriting values: https://docs.python.org/3/library/stdtypes.html#dict.update

I suggest rewriting the docstring:

        Parameters
        ----------
        update : dict
            A mapping used for the state point update.
        overwrite : bool, optional
            If False, an error will be raised if the update modifies the
            values of existing keys in the state point. If True, any existing
            keys will be overwritten in the same way as :meth:`dict.update`.
            Use with caution! (Default value = False).

This also affects Project.update_statepoint.

@bdice bdice added bug Something isn't working documentation Writing or editing documentation labels Feb 17, 2021
@bdice bdice added this to the 1.7.0 milestone Feb 17, 2021
@bdice bdice added the good first issue Good for newcomers label Feb 17, 2021
@cbkerr
Copy link
Member

cbkerr commented Feb 17, 2021

I think we missed what the old docstring meant by: "Set [overwrite] to true to ignore whether this update overwrites parameters [that] are currently part of the job's state point." It didn't address the false case. But now we know!

I'll fix with your change

@stale
Copy link

stale bot commented Apr 18, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 18, 2021
@stale stale bot closed this as completed Jun 2, 2021
@bdice bdice reopened this Jun 3, 2021
@bdice
Copy link
Member Author

bdice commented Jun 3, 2021

This isn't complete but I'm opening a PR to resolve it and will tag @cbkerr for review.

bdice added a commit that referenced this issue Jun 3, 2021
@bdice bdice closed this as completed in #563 Jun 3, 2021
bdice added a commit that referenced this issue Jun 3, 2021
* Resolve #506.

* Update changelog.
@bdice bdice removed the stale label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Writing or editing documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants