What are "side effects" in PR template?

Developers
1

Side effects section of the PR of Stan Math repository only says “Are there any side effects that we should be aware of?”. I always assumed that means side effects of the PR - whether the PR makes any changes that are not requesterd in the issue the PR is solving.

But then in one PR @syclik pointed out that that side effect can also mean a side effect of a function. That is any changes a function does to (global) state of the program. In other words anything a function does that is not just calculating its return value. He than states that all such side effects should also be documented in that PR section.

If that is the case I think the pull reqest template should be updated to be more descriptive about what should go in the side effects section. Maybe something like:

This section should document both:
- any changes made by this PR that were not requested by the referenced issue
- any side effects the functions modified in this PR do (anything function does that iis not just calculating its return value)
3 Likes
2

Sorry for the confusion here. I understand we’re all at different stages of development and we don’t quite share the same terminology. (Especially since terminology like “side effect” has shifted ever so slightly.)

What you’ve described (the two bullet points) was not the intention of the PR template section. Those two bullets should have already been included in the PR template. I don’t know if you’ve seen this guide on How to Write Doxygen Comments that’s in our Stan wiki. I’ve linked to the “What to Document” section. Here’s what it currently says:

What to Document

  • The first sentence, which will be used for a summary in the higher level docs, should be a concise but precise one-sentence statement of what the function returns and what its side effects are if any, based on what the arguments are.
  • Sentences after the first sentence can be used to define terms used in the first sentence, clarify finer behavior, provide example usage, mathematical background, or whatever else you deem necessary.
  • Arguments should be documented by function and use @param[in] , param[out] , or param[in,out] depending on wehther [sic] the parameter must be specified on input, is defined on output, or is specified on input and modified on output.

If that’s followed, the second bullet (“any side effects the functions modified in this PR do…”) should always be documented. As a PR submitter, the first bullet should be described. But honestly, that should be right up front in the “Summary” section of the PR. From the pull request template:

Summary

Describe the contents of the pull request. The issue describes the problem that needs to be fixed, whereas the text here should help the reviewer figure out to what extent this pull request solves the issue.

Describe implementation details and any relevant information that would help the code reviewer understand this pull request. If there is anything special you need to draw someone’s attention to, do it here. Link to any related pull requests or Discourse threads.

Any changes like that really fall under “implementation details” and “relevant information.”

That said, if you think changing the side effects section so it lists that explicitly would help and not restrict the PR submitter from having a place to add additional things, we should do that.

I think there’s a different understanding of the severity of side effects in code. Side effects are a big deal. Especially in an open-source project where we expect there to be a lot of developers that rotate in and out and volunteer their time. The way software scales out is by modularity. Side effects are one of those things that could tie different pieces together. So any time there is a side effect, we really need to ask “is this necessary? is there a different implementation that avoids it?” The existence of side effects may not be avoidable. If it’s not, we have to document it clearly. If I had my way, it’d be in bold and tested thoroughly. But… as a community, we agreed to just document it and have at least some tests for the behavior of the function. That said, side effects must be documented. This isn’t really an option if we’re trying to keep the C++ code maintainable.

And if you’re wondering about all the autodiff code, yes, they all have side effects and interact with the autodiff stack. This isn’t documented at each function because we’ve written about it on the wiki, it’s documented in C++ on the autodiff stack, and it’s also written in the arXiv paper. That’s fundamental to the Math library. Whenever we do something non-standard with the stack, we definitely try to document any side effects clearly. (There is still code that hasn’t been brought up to this standard.) This is why some of the work @wds15 did was so tricky and took so long to evaluate: when it changes how the autodiff stack works, it actually interacts with all our autodiff functions. And we had to evaluate whether the assumptions each of the functions made about the autodiff stack still held. It’s a lot harder to do when it’s not clearly documented, but there is some benefit in brevity and consistency.

Anyway, all that’s to say that side effects matter and it’s preferable to have functions without side effects (also called pure functions). If they are necessary, they need to be documented.

Edit: fixed typo: “for modularity” → “by modularity”

3 Likes