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

Allow HAL to override certain trajectory parameters from starting_config.info dict #4

Merged
merged 8 commits into from
Feb 27, 2023

Conversation

bernstei
Copy link
Collaborator

Issue #3

@casv2
Copy link
Collaborator

casv2 commented Feb 26, 2023

This looks good to me, functionality remains the same yet we add the option to start HAL from a few configs of interest by using the HAL_traj_params, very nice for usability I think.

I'm thinking ahead and would it make sense to trigger a DFT calculation if there are no E/F/V labels/observations for those specific configurations? And maybe some E0 calcs or other method of retrieving the E0s? I basically want people to "just" specify a few configs of interest and automate everything until a good stable potential comes out.

Labelling your data isn't much work ofcourse and I'm not sure we have to do this in this P+R, but it is a (related) thought. I just want to "send" say four unlabelled structures and get an ACE model back without thinking about anything apart from the HAL_traj_params (which already has a few good defaults too).

@casv2
Copy link
Collaborator

casv2 commented Feb 26, 2023

Maybe my comment above is a bit too far fetched for now, I do like to end up there eventually.

@bernstei
Copy link
Collaborator Author

I agree that we could automatically call the reference calculator for anything in fit_configs, but I think that's belongs in a separate PR. Can you open an issue to remind us to do it eventually?

@casv2
Copy link
Collaborator

casv2 commented Feb 26, 2023

Done, merge?

@bernstei
Copy link
Collaborator Author

Done, merge?

Have we actually tested? Do we want to modify/add a test, to make sure it at least runs?

@bernstei
Copy link
Collaborator Author

I added a test, and when I get the github CI to run pytest, I'll merge it into this one and it'll run.

@bernstei
Copy link
Collaborator Author

Tests are running locally. Once they pass, I'll push, and once they pass here, I'll merge.

test_HAL.py passing now, shortened

Change how default basis smoothness_prior hyper is specified, and error
for invalid values.
@bernstei
Copy link
Collaborator Author

@casv2 the tests pass locally, so I hope they'll pass here. Please take a look somewhat carefully at the substantive changes. In particular, I changed how the default basis handles the smoothness_prior hyper, in particular the no prior option and invalid values.

@casv2
Copy link
Collaborator

casv2 commented Feb 27, 2023

This looks great thanks, also thanks for fixing a few pieces of bad code

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.

2 participants