-
Notifications
You must be signed in to change notification settings - Fork 10
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
Make the MACE fit more flexible and build a finetuning workflow #182
Conversation
Update pyproject
docs/user/fitting/fitting.md
Outdated
Currently, this can only be done by cloning the git-repo and installing it from there: []() | ||
|
||
It is now important that you switch off the default settings for the fitting procedure (use_defaults_fitting=False). | ||
If one wants to perform very low-data fine-tuning (less than 4 additional structures), please also think about switching off `preprocessing_data` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuantumChemist can you confirm that this is enough to skip the train, test split and simply take all data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then the MLIPFitMaker will only run if train.extxyz and test.extxyz files are present in the database_dir
autoplex/autoplex/fitting/common/flows.py
Line 182 in fdbee8e
# this will only run if train.extxyz and test.extxyz files are present in the database_dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about a way to circumvent this in the future. I will implement a basic implementation first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was implemented this way by @YuanbinLiu for the RSS part, so we might be careful with the changes until the rest of the RSS part is fully merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I will only add the option not change the default. This is what I have done for the rest of the implementations here as well. The defaults have not changed.
@YuanbinLiu FYI, I will merge this and then push a few tiny changes to #203 to fix the merge conflicts. I have prepared everything in #213 already and this should not cause any problems for your pull request. |
Closes #178