lab-cosmo / metatrain

Training and evaluating machine learning models for atomistic systems.
https://lab-cosmo.github.io/metatrain/
BSD 3-Clause "New" or "Revised" License
13 stars 3 forks source link

Verify for unknown architecture fields #241

Closed PicoCentauri closed 1 week ago

PicoCentauri commented 3 weeks ago

Partly closes #168 since for now I only added a verification for the architecture hypers using a function check_architecture_options. For the checking the dataset it is a bit more complicated because the yaml layout is very flexible. I think it is doable but also to keep the PR a bit smaller I will do this in another run.

While touching the code I also moved the check_options_list inside the dataset expansion because these two functions always will be called together. Also, there was no test if the written option_restart.yaml is actually a valid input to start another training run.

Contributor (creator of pull-request) checklist


📚 Documentation preview 📚: https://metatrain--241.org.readthedocs.build/en/241/

frostedoyster commented 3 weeks ago

Thanks a lot for the work. I tried this branch but I'm still finding some issues in practice:

architecture:
  name: experimental.soap_bpnn
  training:
    batch_size: 2
    num_epoch: 1

training_set:
  systems:
    read_from: qm9_reduced_100.xyz
    length_unit: angstrom
  targets:
    energy:
      key: U0
      unit: eV

test_set: 0.5
validation_set: 0.1

With this config, we would like to see an error (the key is supposed to be num_epochs, not num_epoch). This config results in training for 100 epochs (the default for SOAP-BPNN). No warnings or errors

PicoCentauri commented 3 weeks ago

Interesting let me check this again.

PicoCentauri commented 3 weeks ago

I fixed checking the options you posted @frostedoyster and added them as a test.

frostedoyster commented 3 weeks ago

We seem to have some issues: In the SOAP-BPNN, try adding per_structure_targets: ["energy"] to the training options. This fails and it's not supposed to... Much more concerning: this also fails (also training section of SOAP-BPNN): loss_weights: {"energy": 1.0}, and, with the current design, it's expected to fail. I would propose to ignore dicts that are empty in the reference

Luthaf commented 3 weeks ago

I guess that the issue is that we are trying to use the default hypers as a schema for all possible hypers.

If we want to do proper validation, we should have an actual schema. JSON schema seems to be the main industry standard: https://json-schema.org/. Trying to do validation without a schema, we will always have either things we can't validate, or things we are too strict validating.

However, this would introduce more work for architecture contributors, so it might be worth to discuss it more in depth later. One thing we could have to reduce this work would be some tool to auto-generate the schema from an example YAML. I'm sure something like this already exists.

PicoCentauri commented 3 weeks ago

Yes, I thought we can avoid using a JSON schema but it seems like we can't. I will explore if there is an easy way that is not too much work for developers.

PicoCentauri commented 2 weeks ago

The code and checks looks pretty good, I think we need more documentation about this in the contributors documentation.

Yes sure I can add additional information in the section about adding a new architecture.

Is there a tool that can create a (non-ideal) schema from an example YAML file? We could recommend it as a starting point.

I checked a couple tools in the beginning, but all of them are not great. In the end, I used CHATGPT for generating, which was much better compared the other tools. I think it is okay if we can recommend this.

Luthaf commented 2 weeks ago

In the end, I used CHATGPT for generating, which was much better compared the other tools. I think it is okay if we can recommend this.

We can suggest some tools with their limitations, and say that we also had good success using ChatGPT/LLM for this!

PicoCentauri commented 2 weeks ago

I updated the page for documentation including tools for generating the schemas.

@Luthaf and regarding linking the rascaline schemas: jsonschemas fortunately supports referencing external schemas from local files and URLs. But, this requires some not super easy custom to a Validator class. I played around with this, but I think I need some more time to get this to work in a robust way. If you agree, I will explore this further and add this in an upcoming PR.

Luthaf commented 2 weeks ago

@frostedoyster @abmazitov @DavideTisi @spozdn Can you look at the new json-schema file in your architecture and let us know if (a) you understand the file format and what it is doing; and (b) you agree with the types/constrains of everything?

frostedoyster commented 1 week ago

It looks good to me and the schemas are quite intuitive