MUCollective / multiverse

R package for creating explorable multiverse analysis
https://mucollective.github.io/multiverse/
GNU General Public License v3.0
62 stars 5 forks source link

parent environment for multiverse environments #18

Closed abhsarma closed 4 years ago

abhsarma commented 5 years ago

What should the parent environment to the multiverse environments be (maybe where the multiverse is first created, if that's possible?)

Currently runs in the global environment.

mjskay commented 5 years ago

It seems like one of the issues in #41 stems from the environments that get put into .results. Probably need to:

abhsarma commented 4 years ago

tl;dr in almost all cases, this is not a problem rn; so close?

I don't remember if this ever got resolved. Currently the parent environment is the global environment, and it seems to be working fine in both interactive use and knitting / compiling the documents. I am not sure what might happen if the user creates the multiverse object in an environment itself, but not sure why someone would do something like that?

mjskay commented 4 years ago

If someone makes a multiverse inside a function it would be created in a different environment. I guess the question is if variables should be pulled from where the multiverse is created or where inside() is called?

abhsarma commented 4 years ago

if multiverse object is created in the globalenv, and inside is called from within some function and tries to access a variable declared in the same function ---> does not work In all the other combinations, there isn't a problem. So basically any variable passed into the multiverse using inside() has to be in the same environment as where the multiverse is declared.

The solution to the problem I described here would be to check not just where the multiverse is declared but where the inside is called as well which might be in the same or lower env, but not higher. If that makes sense?

Although, this might be complicated if people use inside in their own function and in the global environment using the same multiverse object

abhsarma commented 4 years ago

I am also not sure how to keep this execution consistent with the multiverse code chunk format

mjskay commented 4 years ago

Hmmmm.

Maybe we could consider it bad practice to go around calling inside() from environments different from where multiverse() is called, since that implies doing some potentially weird things (like creating variables between multiverse code chunk calls but not creating those inside the multiverse itself). So from that perspective, maybe the thing to do is set the parent environment to be the environment the multiverse object was created in?

abhsarma commented 4 years ago

right, then we should probably also check and throw an error if inside is being called from an environment which is not the same as that of the multiverse object?

mjskay commented 4 years ago

hm, maybe? I mean, I can see reasons why that would be okay if someone is trying to build higher-level code against the library. The problem is if code in the inside() call expects to be able to access variables in the environment the call is made in rather than in the environment the multiverse was created in, which would be a hard thing to detect and create an error for. I think it's enough of a corner case that it's probably better to just ignore that potentiality than try to write some complicated check for it.

abhsarma commented 4 years ago

unless I am missing something, this seems to be pretty straightforward using pryr::where?

mjskay commented 4 years ago

That seems like a lot of work for an error message? That is to say, I would place the error message as very low priority (i.e. not CHI beta level) and just fix the multiverse environment part unless it's something that takes, say, 10 minutes to do.

abhsarma commented 4 years ago

i've already implemented it. Although not sure if I am considering all the cases, but it shouldn't take too long to fix if I find new corner cases

mjskay commented 4 years ago

Yeah my main concern is corner cases --- if it could catch things that are not errors then it would be problematic (I'm not really worries about it not catching things that are errors). So long as you can convince yourself that the approach you used won't have false positives (only false negatives) it should be fine.

abhsarma commented 4 years ago