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

More flexible pyimport syntax. Fixes #66 #329

Closed
wants to merge 1 commit into from

Conversation

cstjean
Copy link
Collaborator

@cstjean cstjean commented Nov 21, 2016

No description provided.

@codecov-io
Copy link

codecov-io commented Nov 21, 2016

Current coverage is 63.56% (diff: 88.23%)

Merging #329 into master will increase coverage by 0.49%

@@             master       #329   diff @@
==========================================
  Files            14         14          
  Lines          1259       1279    +20   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            794        813    +19   
- Misses          465        466     +1   
  Partials          0          0          

Powered by Codecov. Last update b050d94...992d8d6

@stevengj
Copy link
Member

How does this handle "merge conflicts", i.e. if the imported function is already defined?

I have a growing sense that @pyimport was a mistake. It is inefficient (because it has to convert everything in the module at import time, and doesn't work with precompilation), it tricks people into thinking that PyCall supports Pythonic dot syntax, and it is awkward (pymember) if the default conversion is not the one that is wanted. I'm thinking it would would be better to deprecate @pyimport and just teach people to use math = pyimport("math") and then use math[:pow] etcetera. If they want to import a single function they can do foo = pyimport("bar")[:foo].

@cstjean
Copy link
Collaborator Author

cstjean commented Nov 21, 2016

@pyimport math: sin
> Cannot @pyimport sin. It is already defined in the current module.

I have a growing sense that @pyimport was a mistake.

I see what you mean. The new syntax doesn't rely on the pywrap trick, at least, when importing functions/classes.

@pyimport sklearn.linear_model: (LinearRegression, LogisticRegression)

I'm OK with taking it out of PyCall, but I will keep it alive elsewhere in my codebase, as I find it rather elegant.

@stevengj
Copy link
Member

We could always support

LinearRegression, LogisticRegression = pyimport("sklearn.linear_model")[:LinearRegression, :LogisticRegression]

by special-casing a tuple of symbols in getindex

@cstjean
Copy link
Collaborator Author

cstjean commented Nov 21, 2016

Another option would be to unexport it and take it out of the README.

@stevengj
Copy link
Member

The migration path would be to @deprecate the pywrap function for a while, so it would continue to work but with a deprecation warning.

@cstjean
Copy link
Collaborator Author

cstjean commented Nov 21, 2016

That's fine for me. I'm already providing this PR's macro in ScikitLearn.jl, because it makes my examples look friendlier for people coming from Python. I can keep doing that.

@stevengj
Copy link
Member

(If Julia would just get dot overloading already then this would be mostly moot, grrr.)

@stevengj stevengj mentioned this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants