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

Update docstring of simDeath() in core. #302

Merged
merged 2 commits into from Jun 16, 2019
Merged

Conversation

pkofod
Copy link
Contributor

@pkofod pkofod commented Jun 13, 2019

Docstring doesn't match behavior.

Docstring doesn't match behavior.
@project-bot project-bot bot added this to Needs Triage in Issues & PRs Jun 13, 2019
@pkofod pkofod changed the title Update docstring of getMortality() in core. Update docstring of simDeath() in core. Jun 13, 2019
@mnwhite
Copy link
Contributor

mnwhite commented Jun 13, 2019

Good catch! But my typo is in the opposite direction: the docstring says what it's supposed to do, but the np.ones is supposed to be an np.zeros. We can also consider removing that print statement and letting "no death and replacement" be the default.

@pkofod
Copy link
Contributor Author

pkofod commented Jun 13, 2019

But my typo is in the opposite direction: the docstring says what it's supposed to do, but the np.ones is supposed to be an np.zeros.

I thought that might be the case, but I couldn't determine if you intended for everyone to die by default and thereby force people to respect the printed message, or if it was a typo the other way. In that case, I will change the behavior! (and add a test), and remove the printed message. Then noone dies, unless the users specifies it.

@llorracc
Copy link
Collaborator

Sounds like "what to do" is now agreed on this? Or is more discussion needed?

@mnwhite
Copy link
Contributor

mnwhite commented Jun 15, 2019 via email

@pkofod
Copy link
Contributor Author

pkofod commented Jun 16, 2019

I removed the warning, and changed the default.

@mnwhite mnwhite merged commit a78df9c into econ-ark:master Jun 16, 2019
Issues & PRs automation moved this from Needs Triage to Done Jun 16, 2019
@pkofod pkofod deleted the patch-5 branch June 16, 2019 20:55
@shaunagm
Copy link
Collaborator

@pkofod, can you write a release note for this?

@pkofod
Copy link
Contributor Author

pkofod commented Jun 28, 2019

"Fix unintended behavior in default simDeath(). Previously, all agents would die off in the first period, but they were meant to always survive."

This change is technically breaking. Before, there would be a warning that the users should define their own method and all agents would die. Now we simply assume that agents live forever, and people have to define a method to avoid this. That said, bug fixes (this had unintended behavior) are not always interpreted as requiring a major version bump under semver 2.0, especially so if no users are expected to rely on the old (wrong) behavior. Since the old method was accompanied by a warning to define a new method, I think we're in the clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Issues & PRs
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants