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

PythonCall extension #53

Merged
merged 5 commits into from
Jun 21, 2023
Merged

PythonCall extension #53

merged 5 commits into from
Jun 21, 2023

Conversation

tjjarvinen
Copy link
Collaborator

@tjjarvinen tjjarvinen commented Jun 20, 2023

This removes PyCall and adds PythonCall as an extension. #51

To use sklearn you need to load both ACEfit and PythonCall

using ACEfit
using PythonCall

There is no need to install sklearn, as that is taken care automatically when PythonCall is loaded.

Notes:

  • Extension does not work with earlier Julia version and probably cause them to fail. We need to disscuss shall we remove support for previous version or try some hacks to get it to work - please let CI to run fully to see if there are errors with old versions.
  • I updated CI a little bit. It still used the old MolSim registry

@cortner
Copy link
Member

cortner commented Jun 20, 2023

Thank you, @tjjarvinen !

@wcwitt -- how do you feel about moving our entire software stack to J1.9? I see no reason at the moment not to. (I want to use the extensions mechanism in other packages as well.)

Teemu - can you think of potential downsides?

@tjjarvinen
Copy link
Collaborator Author

In my opinnion we should go for v1.9. It seem to be general consensus that all packages will switch to it eventually just because of extensions. It is also easier to got for it now that ACEfit is not in general registry.

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 20, 2023

Thanks @tjjarvinen! I don't fully understand J1.9 extensions yet, but I've switched to 1.9 and I have no problem requiring it. Do you want to update the Project.toml and CI accordingly? I don't think I can modify this PR

@tjjarvinen
Copy link
Collaborator Author

I pushed an update for v1.9 and CI for this PR

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 20, 2023

Thanks. Tests pass now - shall I merge?

@cortner
Copy link
Member

cortner commented Jun 20, 2023

I guess this would be breaking and require 0.2? It shall we close our eyes for a moment and tag it as 0.1.1?

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 21, 2023

I vote 0.1.1 for now.

@tjjarvinen
Copy link
Collaborator Author

I vote for 0.1.1 too. Only thing that changed is that now you need to load PythonCall instead of PyCall. All else works as it was before, except that you need to have Julia v1.9 or newer.

@cortner
Copy link
Member

cortner commented Jun 21, 2023

great, I will merge and tag.

@cortner cortner merged commit a800e1e into ACEsuit:main Jun 21, 2023
7 checks passed
@cortner
Copy link
Member

cortner commented Jun 21, 2023

this is now registered as 0.1.1

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 22, 2023

@tjjarvinen, I just realized that src/solvers_pycall.jl still exists. Is this something we should delete now that ext/ACEfit_PythonCall_ext.jl exists?

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 22, 2023

Or maybe we just need to change the name to src/solvers_pythoncall.jl?

@tjjarvinen
Copy link
Collaborator Author

Ye, I forgot to rename that file.

Basicly you need to define the solver structs for sklearn there, so that you can use the extension to overload solve. I think the best option is to rename the file to something like sklearn_structs etc. That should make it clear for future use.

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 22, 2023

I think I would vote to keep the filename analogy with solvers.jl. Happy with either solvers_pythoncall.jl or solvers_sklearn.jl. And we add a comment pointing to the extension?

Edit: actually could we just move them back to solvers.jl and there have the comment about the extension?

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 22, 2023

I have done the latter in 9f4e744

@cortner
Copy link
Member

cortner commented Jun 26, 2023

anything for me to do here? test and tag another version?

@wcwitt
Copy link
Collaborator

wcwitt commented Jun 27, 2023

You are welcome to tag a new version if you like, but its just a name change so I'm also fine holding off for now

@tjjarvinen tjjarvinen mentioned this pull request Jun 28, 2023
2 tasks
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