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

WIP: New Affine Model Interface #92

Merged
merged 7 commits into from
Jan 21, 2023
Merged

WIP: New Affine Model Interface #92

merged 7 commits into from
Jan 21, 2023

Conversation

cortner
Copy link
Member

@cortner cortner commented Jan 16, 2023

This implements the proposed affine model interface. @tjjarvinen and I think this will make our models far more accessible from Julia and maybe this can also guide simplification of the python/json interface.

Here is what a minimal script now looks like:

using ACE1pack
data = read_extxyz(joinpath(ACE1pack.artifact("TiAl_tutorial"), "TiAl_tutorial.xyz"))
model = acemodel(species = [:Ti, :Al], N = 3, maxdeg = 15, rcut = 5.5, 
                 maxdeg2 = 10, Eref = Dict(:Ti => -1586.0195, :Al => -105.5954))
acefit!(model, data[1:5:end], ACEfit.RRQR(rtol = 1e-4))
# export_ace("TiAl.yace", model)

My goal is that, by the time the software paper is published, these few lines give a very good, robust model with repulsive core.

The last line is commented because this doesn't work yet. @casv2 and @WillBaldwin0 -- can you please coordinate providing this functionality? I suggest that we merge this first and you can do it in a separate PR.

@cortner cortner requested a review from wcwitt January 16, 2023 00:53
@cortner
Copy link
Member Author

cortner commented Jan 16, 2023

@wcwitt -- should we use a different solver as the default?

@cortner
Copy link
Member Author

cortner commented Jan 19, 2023

@wcwitt --- sorry to ping you again. Can you look at the minimal script above and suggest what the default solver should be?

@wcwitt
Copy link
Collaborator

wcwitt commented Jan 19, 2023

Sorry - will do a full review as soon as possible. For the solvers, is there a reason you chose RRQR? ACEfit has a few Bayesian solvers, if that's what you actually want, and I think BLR could be reasonable as a default also.

I think I need to rename some things - right now I have slightly different names for BRR-variants using different factorization strategies (e.g. SVD or Cholesky), and things aren't as clear as they should be

@cortner
Copy link
Member Author

cortner commented Jan 19, 2023

BLR or ARD and any parameters?

@wcwitt
Copy link
Collaborator

wcwitt commented Jan 19, 2023

I would say BLR (it's notably faster than ARD) with default parameters.

But linking this issue ACEsuit/ACEfit.jl#41.

@gelzinyte
Copy link
Collaborator

I don't think there will be much of a change in the user-facing side of ACE1pack-julia/json interface, because all of the same parameters still need to be specified (but let me know if I miss something here). But how ACE1pack parameters are translated into acemodel will need to be updated, I'll wait for this to be merged first.

@cortner
Copy link
Member Author

cortner commented Jan 19, 2023

@gelzinyte --- I'm not even so sure that this new model interface will change much (or anything?) for the JSON interface. What do you think?

@gelzinyte
Copy link
Collaborator

Yes, I agree. I was talking about switching to using acemodel in ACE1pack.fit_ace, for example. I don't think that will change too much either.

@cortner
Copy link
Member Author

cortner commented Jan 20, 2023

add alternative tutorials that use this new interface, then merge it.

@cortner cortner merged commit 1a97df6 into main Jan 21, 2023
@cortner cortner deleted the co/model branch January 21, 2023 05:05
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