Conversation
ElliottKasoar
added
the
new benchmark
|
PET-MAD seems to fail sometimes. It failed in one run but if I run it again with the same setting it runs without problem. |
ElliottKasoar
left a comment
ElliottKasoar
left a comment
There was a problem hiding this comment.
Thanks for this, @jungsdao!
Apologies for the slow review.
I'd be inclined to rename the test something slightly more specific e.g. OC20NEB, since this name can't clash with any other test.
I'd also like to explore replacing a lot of the calculation with janus-core's NEB calculation, as it's a lot of duplication, and I think the only thing missing is a way of checking convergence/continuing, both of which should be minor additions.
| Surface reaction | ||
| ============ |
There was a problem hiding this comment.
| Surface reaction | |
| ============ | |
| Surface reaction | |
| ================ |
This needs to be at least as long as the title
There was a problem hiding this comment.
Could you please send these as a zip file ([benchmark_name].zip) to me and/or @joehart2001 so we can upload them to the S3 bucket, and then update your calculation to download them?
| #'desorption_ood_249_10303_1_111-1', | ||
| # 'desorption_id_240_3075_0_111-1', | ||
| # 'desorption_id_182_8153_14_011-5', | ||
| # 'desorption_id_181_11171_3_100-1', | ||
| # 'desorption_id_150_10251_16_211-2', | ||
| # 'desorption_id_263_4981_0_211-1', | ||
| # 'desorption_id_258_8832_5_200-0', | ||
| # 'desorption_id_15_10898_8_122-6', | ||
| # 'desorption_id_153_9859_14_100-0', | ||
| # 'dissociation_ood_71_9489_25_000-3', | ||
| # 'dissociation_id_281_2644_33_200-4', | ||
| # 'dissociation_ood_393_8094_1_100-6', | ||
| # 'dissociation_id_526_4029_53_200-2', | ||
| # 'dissociation_id_154_1581_1_111-0', | ||
| # 'dissociation_id_628_3261_0_100-1', | ||
| # 'dissociation_id_397_164_30_100-0', | ||
| # 'dissociation_id_181_116_6_111-0', | ||
| # 'dissociation_ood_205_2717_39_111-5', | ||
| # 'transfer_ood_578_10006_4_200-1', | ||
| # 'transfer_ood_594_10582_21_100-2', | ||
| # 'transfer_id_610_1068_2_100-2', | ||
| # 'transfer_id_641_8304_21_011-2', | ||
| # 'transfer_id_295_6103_4_211-0', | ||
| # 'transfer_id_171_9735_4_222-0', | ||
| # 'transfer_ood_567_3215_0_111-0', | ||
| # 'transfer_ood_519_6938_8_200-5', | ||
| # 'transfer_id_306_9788_21_111-0', |
There was a problem hiding this comment.
Is the idea here that all of these files will be used? For the purpose of this PR, it would be good to set it up as the 'full' version. If these aren't available, I'd delete these
Given that ideally these will be files in a zip file we access, we probably don't even need to list them out, as we can just take the file names from the relevant directory?
|
|
||
| @pytest.mark.slow | ||
| @pytest.mark.parametrize("model_name", MODELS) | ||
| def test_surface_reaction3( |
There was a problem hiding this comment.
Is the only difference between these tests the input file? If so, I suggest parameterising the test in a similar way to how we parameterise MODELS (it will be combinatorial if you add it as a second @pytest.mark.parametrize input)
|
|
||
| for struct in [initial, final]: | ||
| struct.calc = calc | ||
| struct.constraints = [FixAtoms(indices=fix_indices)] |
There was a problem hiding this comment.
There were various bugs relating to this in previous versions of ASE, but does the move_mask not correspond to the same thing as tags, and I think ASE can automatically can apply the FixAtoms constraint from this?
|
|
||
| # model.device = "cuda" |
There was a problem hiding this comment.
| # model.device = "cuda" |
| desorption_ood_87_9841_0_111-1 barrier error: | ||
| good: 0.1 | ||
| bad: 0.5 | ||
| unit: eV | ||
| tooltip: "desorption_ood_87_9841_0_111-1 barrier error" | ||
| level_of_theory: RPBE | ||
| dissociation_ood_268_6292_46_211-5 barrier error: | ||
| good: 0.1 | ||
| bad: 0.5 | ||
| unit: eV | ||
| tooltip: "dissociation_ood_268_6292_46_211-5 barrier error" | ||
| level_of_theory: RPBE | ||
| transfer_id_601_1482_1_211-5 barrier error: | ||
| good: 0.1 | ||
| bad: 0.5 | ||
| unit: eV | ||
| tooltip: "transfer_id_601_1482_1_211-5 barrier error" |
There was a problem hiding this comment.
Are there more descriptive/human readable names for these?
| MODELS = get_model_names(current_models) | ||
| BENCHMARK_NAME = "Surface reaction" | ||
| DOCS_URL = ( | ||
| "https://ddmms.github.io/ml-peg/user_guide/benchmarks/nebs.html#surface-reactionn" |
There was a problem hiding this comment.
| "https://ddmms.github.io/ml-peg/user_guide/benchmarks/nebs.html#surface-reactionn" | |
| "https://ddmms.github.io/ml-peg/user_guide/benchmarks/nebs.html#surface-reaction" |
Assuming we keep the test name
|
Actually from the last drop-in meeting, there was discussion that we can modify this benchmark similar to bulk-crystal Phonon. So I can add more reactions from OC20NEB and make a scatter plot. (I'm not sure yet how many reactions are feasible yet.) I'll try to incorporate |
Great! Please let us know if there any issues. I've recently proposed some changes to Also just to note you may need to rebase/merge the latest |
2 participants
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
Add NEB benchmark for three surface reactions from OC20NEB dataset comparing barrier height errors.
Linked issue
Resolves #293
Progress
Testing
Test on GPU with
New decorators/callbacks
None