-
Notifications
You must be signed in to change notification settings - Fork 8
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
Save model info #336
base: main
Are you sure you want to change the base?
Save model info #336
Conversation
we need to think about this one, what happens if we have two calculations, both arch=mace but model=small and model=large, how will the extxyz look and similarly if we have some models passed by paths(do we get only the basename for mlip?). I think will be nice to document all this structure of what we want and then review the code accordingly |
65cd5f1
to
f2f38c5
Compare
Good question. At the moment I think having the same E.g. When we change architecture, this is a bit more complicated, since we'd change E.g. I implemented it this way for now since it means We could change both to potentially be lists (if more than one value), either as two individual lists or a combined entry This does make things more complicated though, since we'd then want to do something like convert repeated
Regarding paths, at the moment model paths are saved as |
ok I think we need to document very well the behaviour of this with examples since there is no obvious solution. |
f2f38c5
to
4b2fc67
Compare
4b2fc67
to
778e777
Compare
Co-authored-by: Jacob Wilkins <[email protected]>
b186c38
to
ee72b37
Compare
Resolves #327
For
model_path
(or equivalent inputs e,g,model
), this addsmlip_model
as an info label when structures are output.For strings and paths, this is fine. For already-loaded torch models, it's less clear what the best label is, so I'm open to alternatives.
This will also interact with any implementation of #240, but we can deal with that when we come to it.
@alinelena, was the suggested
info['{arch}_{model}'] = model
replacinginfo["arch"]
or additional? To me it doesn't make sense to have"{arch}_model"
as well as"arch"
, which is how I originally interpreted it, but perhaps it makes sense to combine them into a single info label, which I just realised may be what you meant.