pairs.R: evaluate in parent.frame() (maybe)

[jgabry]

Summary:

Fixes #1155, although this might not be the preferred solution. This PR changes pairs.stanfit so evaluation takes place in the calling environment, which avoids the error in #1155. However, this approach could have unintended consequences, so I'm not sure whether we should actually do this.

Copyright and Licensing

Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Columbia University

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses:

• Code: GPLv3 (https://opensource.org/licenses/GPL-3.0)
• Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

[iago-lito]

Just to make sure I understand this change correctly. We would transition from:

value <- 5
for (value in 1:3) {
   pairs(fit, arg = value) # <- `value` evaluates to global scope: 5 then 5 then 5.
}

to

value <- 5
for (value in 1:3) {
   pairs(fit, arg = value) # <- `value` evaluates to local scope: 1 then 2 then 3.
}

Is that correct?

If so, then I have a strong opinion that it is the correct fix.

To state this further, and considering that it is a breaking change: my opinion is that any behaviour broken by this change should be retrospectively considered a bug. I think that lexical scoping is a standard in all modern languages because it is the most intuitive to expect. The current behaviour can be the source of numerous, subtle, very hard to diagnose bugs. I think that it is a mistake that R even allows this behaviour with its (too) flexible evaluation control opportunities.

Fortunatly this is only about plotting, and it only impacts cosmetic arguments passed to pairs.stanfit so the stakes are rather low. I don't expect much science to be broken by this quirk ;)

Also worth considering: should we keep the current behaviour, then how would we achieve a loop like the second one above?

[jgabry]

Is that correct?

Yes, I think that's right.

To state this further, and considering that it is a breaking change: my opinion is that any behaviour broken by this change should be retrospectively considered a bug.

I agree.

Also worth considering: should we keep the current behaviour, then how would we achieve a loop like the second one above?

Yeah, I don't think it would be possible.

[bgoodri]

I'll test this and presumably roll it into the release we need to do for unrelated reasons.

[andrjohns]

These changes were included in the CRAN update, so merging - thanks for finding and reporting this @iago-lito!