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

ENH: Expose patsy eval_env to users. #2031

Merged
merged 4 commits into from Oct 8, 2014

Conversation

Projects
None yet
3 participants
@jseabold
Copy link
Member

commented Oct 7, 2014

No description provided.

@bashtage

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2014

Maybe patsy_ns or patsy_env to make it clearer that this is for Patsy?

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2014

Yeah I'm not totally sold on the name. I thought about just namespace_depth but it's too long. Does every user know that patsy evaluates the formula? I was assuming not, so that might not clear things up.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Oct 7, 2014

Since it is only used in from_formula I don't see any problem with the name. It should be clear from the context

We don't refer to patsy by name, we usually call everything just formula, e.g. formula_env

@bashtage

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2014

formula_ns or formula_env sound cleaner. eval_env seems really ambiguous since lots of things might be evaluated.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2014

I don't feel strongly about this, but the keyword is called eval_env in the patsy documentation we point to and you could end up passing a EvalEnvironment instance to to it, so it is consistent this way. It's a bit redundant I think to need to specify formula twice, so maybe even just env?

OTOH, is there a case to give the user full control over the evaluation depth in library code? We can just have use_namespace=True and for the power users who understand the implications could overload this with an EvalEnvironment instance?

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2014

So...decision time?

@bashtage

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

When it doubt just leave it...

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2014

Heh, I can get behind that.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Oct 8, 2014

I'm fine either way.
It will be an exotic, unused option for almost all users, I guess

jseabold added a commit that referenced this pull request Oct 8, 2014

Merge pull request #2031 from jseabold/patsy-env
ENH: Expose patsy eval_env to users.

@jseabold jseabold merged commit 47fde88 into statsmodels:master Oct 8, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details

@jseabold jseabold added this to the 0.6 milestone Oct 8, 2014

@jseabold jseabold deleted the jseabold:patsy-env branch Oct 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.