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

using inverse of gap_fit error as weights #19

Open
noambernstein opened this issue May 3, 2021 · 11 comments · May be fixed by #22
Open

using inverse of gap_fit error as weights #19

noambernstein opened this issue May 3, 2021 · 11 comments · May be fixed by #22

Comments

@noambernstein
Copy link

It would be nice if ipfitting could support (more or less) the syntax of gap_fit for its energy/force/virial error, interpreting the weight as the inverse of the error. For this, there would be a global set of default errors, and an optional per-configuration value in the xyz info field.

@bernstei
Copy link

bernstei commented May 3, 2021

The command line arguments are documented here. They are not very relevant, since I'm just talking about the julia fitting functions so far, and a fitting "executable" can do whatever it wants to convert from command line arguments to julia function arguments.

More importantly (and not documented on that page), the per-configuration is in four separate info fields, energy_sigma, force_sigma, virial_sigma and hessian_sigma.

@cortner
Copy link
Member

cortner commented May 3, 2021

that should be enough to get us going - thank you!

@wcwitt
Copy link
Collaborator

wcwitt commented Dec 10, 2021

Christoph pointed me here as a managable first contribution... I'll look into it over the weekend. If the situation has changed in the meantime, just let me know! @bernstei @cortner

@cortner
Copy link
Member

cortner commented Dec 10, 2021

I think step 1 is understanding the difference how GAP and ACE weight the training data. Then it should be easy to implement the translation. For ACE look at these:

https://arxiv.org/pdf/1911.03550.pdf
https://arxiv.org/pdf/1910.06010.pdf. (not ACE, but done the same way)

@wcwitt
Copy link
Collaborator

wcwitt commented Dec 13, 2021

Having looked into it, it seems there are at least two separate tasks.

  1. Modify lsqfit so that it accepts sigmas as a keyword in addition to weights, where the relationship is w=1/sigma^2. (Probably should be mutually exclusive to prevent mixing and matching?)
  • I note gap_fit has separate default_sigma and config_type_sigma options, whereas lsqfit only has weights, which includes the default.
  1. Modify read_xyz so that it can extract per-config weights/sigmas from xyz files.

Does that sound right to start? If so, I'll begin a pull request and we can continue discussing there.

@cortner
Copy link
Member

cortner commented Dec 13, 2021

yep, that sounds right to me. But make sure that the weights are really 1/sig^2 and not 1/sig? @casv2 ???

@wcwitt
Copy link
Collaborator

wcwitt commented Dec 14, 2021

Ah you're right it's w^2=1/sig^2, thanks.

Last question: on what branch should I work? Is the DEV-v0.8.x business contained to the main ACE repo, or is there a similar split here?

@cortner
Copy link
Member

cortner commented Dec 14, 2021

Yes you should start from dev-v0.8.x and merge back to that branch

@wcwitt wcwitt linked a pull request Dec 16, 2021 that will close this issue
@bernstei
Copy link

@cortner says this is in the latest IPFitting, but needs to be back-ported to the v0.10 (which is the ACE1-compatible successor to v0.4) to be available for ACE1, which I'd like.

@wcwitt
Copy link
Collaborator

wcwitt commented Jan 24, 2022

I think what's necessary is for me to tidy #22 and rebase those changes onto several places, including v0.10.

@bernstei, how flexible should things be? Can I restrict the user to providing only weights or only inverse weights? For the per-config values, should there be an energy_weight_key and/or energy_sigma_key analagous to energy_key?

@bernstei
Copy link

bernstei commented Jan 24, 2022

Certainly for any given config I'm happy to require that only one of weights and inverse weights are specified. That'd be OK for the entire set, too, but it's more restrictive and I'd be predisposed to not require that unless it really simplifies something. gap_fit uses energy_sigma, force_sigma (or maybe "forces"), etc as the keys. If I could fit the same fitting databases to ACE that'd be great, so let me know if you need the precise syntax.

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 a pull request may close this issue.

4 participants