ACEsuit / ACEfit.jl

Generic Codes for Fitting ACE models
MIT License
7 stars 7 forks source link

PythonCall extension #53

Closed tjjarvinen closed 1 year ago

tjjarvinen commented 1 year ago

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:

cortner commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

wcwitt commented 1 year ago

Thanks. Tests pass now - shall I merge?

cortner commented 1 year ago

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 commented 1 year ago

I vote 0.1.1 for now.

tjjarvinen commented 1 year ago

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 commented 1 year ago

great, I will merge and tag.

cortner commented 1 year ago

this is now registered as 0.1.1

wcwitt commented 1 year ago

@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 commented 1 year ago

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

tjjarvinen commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I have done the latter in 9f4e7444bbdb2168f1d0e206e79a00f16306cfac

cortner commented 1 year ago

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

wcwitt commented 1 year ago

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