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

Remove set_default_dtype and get_default_dtype #155

Merged
merged 10 commits into from
Mar 26, 2024
Merged

Conversation

PicoCentauri
Copy link
Contributor

@PicoCentauri PicoCentauri commented Mar 15, 2024

Fixes #148

This PR removes all calls of set_default_dtype and get_default_dtype. For now I am using the .to() method to move the model to the correct dtype. But is there also an option to initialize the model in the first place in the correct dtype?

The reveiled some issues that are also fixed within this PR

  1. if a user changes the dtype for continue training the soap-bpnn would not work. Now the dtype of the model is set to the one of the dataset.
  2. similar for eval. There we adjust the loading dtype to the targets to the dtype the exported model

Additional changes

  • changed the default precision to 32 to match the torch one. We can also leave this at 64-bit?
  • replaced assert torch.allclose by the more verbose torch.testing.assert_close

TODO

  • add an eval test for 64-bit and 32-bit exported models
  • add __architecture_capabilities__ to each architecture indicating their supported types
  • add dtype check to check_datasets function

📚 Documentation preview 📚: https://metatensor-models--155.org.readthedocs.build/en/155/

@PicoCentauri PicoCentauri changed the title Remove set_default_dtype and get_default_dtype Remove set_default_dtype and get_default_dtype Mar 15, 2024
@PicoCentauri PicoCentauri force-pushed the explicit-dtype branch 7 times, most recently from af72695 to a7f21d7 Compare March 20, 2024 15:30
Copy link
Collaborator

@frostedoyster frostedoyster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes

base_precision: 64
base_precision: 32
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhhh are we sure we want this to be the default behavior? People have had issues with float32 MD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just changed this to be consistent with torch. Which people had issues? The examples seems to be good and usually problems in MD appear if the forces are not summed up in double precision but as a base precision I think a normal float should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can discuss this together with other people. I still think float64 is a safer default

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using the power of omegaconf's custom resolvers to obtain the preferred dtype for each architecture. When we are happy with this I will do the same for the device (but in a different PR)

src/metatensor/models/cli/eval.py Show resolved Hide resolved
src/metatensor/models/cli/train.py Outdated Show resolved Hide resolved
src/metatensor/models/experimental/pet/__init__.py Outdated Show resolved Hide resolved
src/metatensor/models/utils/data/dataset.py Show resolved Hide resolved
tests/cli/test_eval_model.py Show resolved Hide resolved
Copy link
Collaborator

@frostedoyster frostedoyster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@frostedoyster frostedoyster merged commit dd4f0df into main Mar 26, 2024
11 checks passed
@frostedoyster frostedoyster deleted the explicit-dtype branch March 26, 2024 20:40
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.

Better dtype handling
3 participants