Feat/add folmsbee conformer benchmark#429
Conversation
|
Hi @lwehrhan, thank you for your PR and its looking great overall! A few things:
Once we've got the data file uploaded, i think we can make a few changes to the calc script for consistency with similar benchmarks, but i think the changes will be minor. Just a note, make sure you to fetch any changes i've made before working locally, otherwise your next push may overwrite my changes. thanks! |
|
Hey @lwehrhan, we've just merged a PR so that you can tag your benchmark with the mlip-auditl, add your logo and have your own dedicated tab (PR #434). Please see the framework credit tags docs |
e03bf30 to
efb1ea1
Compare
…b.com/lwehrhan/ml-peg into feat/add-folmsbee-conformer-benchmark
| i = int(conf_str) | ||
| molecule = result_by_name[mol_name] | ||
|
|
||
| results[model_name].append(float(molecule.predicted_energy_profile[i])) |
There was a problem hiding this comment.
This might be None if a molecule had an unsupported element.
There was a problem hiding this comment.
This would be an actual problem in this case. I think we will have to drop the molecules with unsupported elements or skip the benchmark entirely in that case (as in mlip audit). We cannot test for supported elements because the calculators do not expose the supported elements.
There was a problem hiding this comment.
I tried to address this a little in the my comment below about filtering, but I think this is fine - if a model is unable to do the test because it's missing any elements required, ML-PEG would set the score to None/NaN as well.
I think the only thing we'd need in terms of filtering is similarly to how you build the labels currently, if you're also able to get the full set of elements, save this in an info.json file, and pass it to the app, so we can enable filtering for this benchmark?
It doesn't need to be a list per structure, since we don't explicitly loop through them in ML-PEG, so unless the per-structure data is accessible somehow to reevaluate the score, there's no benefit to the future plan for "partial" filtering, and it'll be a binary "is this element present in any of the structures or not" - if is is contained, then the benchmark can have "failed" without impacting the overall score for all the benchmarks.
|
Hey @lwehrhan what’s the status on the unresolved comments? I’ll tag @ElliottKasoar so he can take a look over the benchmark |
|
@joehart2001 I have addressed the comments now. The only open issue (this will also apply to some of the other benchmarks) is that we cannot test the supported elements of the models prior to running the benchmark like we do in mlip audit, which sometimes may lead to bugs. I saw this is handled "on-the-fly" e.g. in |
Hi @leonwehrhan, can you please take a look at the (very new) guidance for element filtering: https://ddmms.github.io/ml-peg/developer_guide/filter.html. Essentially, in an ideal world, we'd catch any errors that occur during calculations, so you shouldn't need to know what calculators support, but have an awareness that they failed for certain molecules, which could be due to element support or otherwise. Then in the analysis, if it fails for any of the molecules, the model can't do the benchmark, so it would get a score of If it wouldn't be possible to set things up to do this partial filtering (e.g. if you can't catch errors and continue for other molecules), it's also fine to only implement the current 'binary' filter, but we would still rather say a model can't do a benchmark than have the scores represent different things for different models. The diatomics example is probably not the best guide, since it came before we added filtering, and so the approach is not quite as consistent. |
|
Given how many points are in the scatter plot, I also wonder if we may want to use the density scatter instead? I think beyond a handful of models the plots will get very crowed otherwise? |
|
Also note there is now a small conflict due to changes we've made to frameworks.yml. Please could this be resolved? |
my understanding is that this would mean dropping the per-conformer structure viewer? |
we can still visualise structures with the denity plots (check e.g. elasticity benchmark), but we could also use the |
|
I think its looking nearly ready from my side of things. thanks for contributing this! |
| "mattersim==1.2.2", | ||
| ] | ||
| mlipaudit = [ | ||
| "mlipaudit; python_version >= '3.11'", |
There was a problem hiding this comment.
dm-tree (via the mlipaudit extra: mlipaudit -> mlip -> dm-tree) ships no Python 3.13 wheel so we get an error for this with 3.13
| "mlipaudit; python_version >= '3.11'", | |
| "mlipaudit; python_version >= '3.11' and python_version < '3.13'", |
|
|
||
| import pytest | ||
|
|
||
| from ml_peg.calcs.utils.mlipaudit import MlPegConformerSelectionBenchmark |
There was a problem hiding this comment.
if mliap audit is not installed and ml_peg calc is run then we will fail on this benchmark so we can skip this more gracefully
| from ml_peg.calcs.utils.mlipaudit import MlPegConformerSelectionBenchmark | |
| # Optional extra (ml-peg[mlipaudit]); skip if not installed. | |
| pytest.importorskip("mlipaudit") | |
| from ml_peg.calcs.utils.mlipaudit import ( # noqa: E402 | |
| MlPegConformerSelectionBenchmark, | |
| ) |
| from mlipaudit.benchmarks.conformer_selection.conformer_selection import ( | ||
| ConformerSelectionModelOutput, | ||
| ) |
There was a problem hiding this comment.
| from mlipaudit.benchmarks.conformer_selection.conformer_selection import ( | |
| ConformerSelectionModelOutput, | |
| ) |
| ) | ||
| from ml_peg.app import APP_ROOT | ||
| from ml_peg.calcs import CALCS_ROOT | ||
| from ml_peg.calcs.utils.mlipaudit import MlPegConformerSelectionBenchmark |
There was a problem hiding this comment.
| from ml_peg.calcs.utils.mlipaudit import MlPegConformerSelectionBenchmark |
| from ml_peg.calcs.utils.utils import download_s3_data # noqa: F401 | ||
| from ml_peg.models import current_models | ||
| from ml_peg.models.get_models import load_models | ||
|
|
There was a problem hiding this comment.
| # Optional extra (ml-peg[mlipaudit]); skip if not installed. | |
| pytest.importorskip("mlipaudit") | |
| from mlipaudit.benchmarks.conformer_selection.conformer_selection import ( # noqa: E402 | |
| ConformerSelectionModelOutput, | |
| ) | |
| from ml_peg.calcs.utils.mlipaudit import ( # noqa: E402 | |
| MlPegConformerSelectionBenchmark, | |
| ) | |
There was a problem hiding this comment.
is it possible to add some form of progress bar?
| dict | ||
| Mapping of model name to ``(benchmark, ConformerSelectionResult)``. | ||
| """ | ||
| data_input_dir = download_s3_data( |
There was a problem hiding this comment.
do we need to download this again?
| i = int(conf_str) | ||
| molecule = result_by_name[mol_name] | ||
|
|
||
| results[model_name].append(float(molecule.predicted_energy_profile[i])) |
| Computational cost | ||
| ------------------ | ||
|
|
||
| Medium: tests may take hours on CPU. |
There was a problem hiding this comment.
what about gpu? also we need to improve our guidance on this but usually we report the time for a single model to be benchmarked, so this would be on the order of minutes/tens of minues for cpu
| Metrics | ||
| ------- | ||
|
|
||
| 1. Conformer energy error |
There was a problem hiding this comment.
update this to include the MAE and conformer score as two separate metrics as in the metrics.yml
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
The Folmsbee dataset of low-energy conformers of drug-like molecules. The differences in energy are smaller compared to the Wiggle500 dataset and it features a greater number of molecules. The highest available level of theory for energy evaluations to be used as ground-truth is DLPNO-CCSD(T). This is a test for moving the benchmarks of mlip-audit into this repository. I have included an analysis script for this benchmark, however would like to kindly ask for assistance with building and harmonizing the Dash layout.
Linked issue
Resolves #427
Progress
Testing
New decorators/callbacks