pass 'data' to parse_data() (fixes bug if data is named "i")

[jgabry]

fixes #654

Summary:

Data named i was overwritten by RStan.

Intended Effect

Give parse_data() a data argument instead of using with(data, ...) in sampling(). And then use mget() instead of dynGet() inside of parse_data(). This avoid overwriting i, but it's unclear why it was being overwritten. See comments in #654.

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 (http://opensource.org/licenses/GPL-3.0)
• Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)

[bgoodri]

Then we must have another loop over i somewhere in scope.

…

On Thu, Jun 27, 2019 at 5:26 PM Jonah Gabry ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In rstan/rstan/R/misc.R <#655 (comment)>: > objects <- intersect(objects, in_data) - stuff <- sapply(objects, simplify = FALSE, FUN = dynGet, - inherits = FALSE, ifnotfound = NULL) + if (is.null(data)) { + # get them from the calling environment + stuff <- sapply(objects, simplify = FALSE, FUN = dynGet, + inherits = FALSE, ifnotfound = NULL) + } else { + # get them from 'data', and we know already from sampling() and other + # methods that is.list(data) & !is.data.frame(data) + stuff <- mget(objects, envir = as.environment(data), inherits = FALSE, + ifnotfound = rep(list(NULL), length(objects))) + } for (i in seq_along(stuff)) if (is.null(stuff[[i]])) { I think it's more complicated than that. Before changing the mget the value of i was already messed up in the stuff object *before* it ever got to the loop part for (i in seq_along(stuff)) .... — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#655?email_source=notifications&email_token=AAZ2XKS7BZ6JE2B4IZCDTDLP4UWAZA5CNFSM4HV6SBU2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB45HKAI#discussion_r298376821>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ2XKU72KSCHKG2ZNXGI5TP4UWAZANCNFSM4HV6SBUQ> .

[jgabry]

I assume so, but not it's not clear where. parse_data is called from the sampling() method which doesn't have an explicit loop over i. And neither does the stan function when it calls sampling.

[jgabry]

If I change parse_data to do the same thing in a loop that sapply was doing (and also use int instead of i to index) then it seems to be ok:

# current method: stuff$i will have wrong value
  stuff <- sapply(objects, simplify = FALSE, FUN = dynGet, 
                  inherits = FALSE, ifnotfound = NULL)

# stuff$i has correct value
  for (int in seq_along(objects)) {
   stuff[[objects[int]]] <- dynGet(objects[int], inherits = FALSE, ifnotfound = NULL)
  }

[bgoodri]

ok

…

On Thu, Jun 27, 2019 at 6:01 PM Jonah Gabry ***@***.***> wrote: If I change parse_data to do the same thing in a loop that sapply was doing (and also use int instead of i to index) then it seems to be ok: # current method: stuff$i will have wrong value stuff <- sapply(objects, simplify = FALSE, FUN = dynGet, inherits = FALSE, ifnotfound = NULL) # stuff$i has correct value for (int in seq_along(objects)) { stuff[[objects[int]]] <- dynGet(objects[int], inherits = FALSE, ifnotfound = NULL) } — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#655?email_source=notifications&email_token=AAZ2XKX6L4BZQMZ3UTWK4C3P4U2CBA5CNFSM4HV6SBU2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYYP5LY#issuecomment-506527407>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ2XKRN45G36D3ARZS4NQDP4U2CBANCNFSM4HV6SBUQ> .