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

Better dtype handling #148

Closed
frostedoyster opened this issue Mar 14, 2024 · 4 comments · Fixed by #155
Closed

Better dtype handling #148

frostedoyster opened this issue Mar 14, 2024 · 4 comments · Fixed by #155
Assignees

Comments

@frostedoyster
Copy link
Collaborator

At the moment, we have that all the readers have a dtype argument with the torch.float64 default. However, with our current global dtype model, this does not make much sense and it makes maintaining the tests quite difficult at times (see #146). I would propose to remove the dtype argument and always use the global dtype, so as to make the logic consistent with our current dtype handling. I do not see it breaking our code in any way, because we always use the global dtype.
This could be changed in the future.
@PicoCentauri

@PicoCentauri
Copy link
Contributor

Personally, I don't like the global dtype option. I know that you will argue that this standard and everybody is doing it, but I think this is an antipattern. Using a global variables makes code very implicit and very hard to debug if something goes wrong. I don't know what you saw in #146 but this seemed unrelated to the dtype.

I would propose to remove the dtype argument and always use the global dtype, so as to make the logic consistent with our current dtype handling.

I would rather say we remove the set global dtype otions and models should use the dtype they get from the dataset. The current implementation in mixed and was a compromise we did.

@Luthaf
Copy link
Member

Luthaf commented Mar 14, 2024

I agree with @PicoCentauri: if we allow setting global dtype, this mean that importing architecture_1 changes the behavior of architecture_2, which I would rather not have.

We can add a lint rule to forbid the use of torch.set_default_dtype() and torch.get_default_dtype() in metatensor-models, but we should have a review point that these are not used in external dependencies either.

@PicoCentauri
Copy link
Contributor

PicoCentauri commented Mar 14, 2024

I will make the dtypes explicit everywhere. @Luthaf do you know how to do such a lint rule?

@PicoCentauri PicoCentauri self-assigned this Mar 14, 2024
@Luthaf
Copy link
Member

Luthaf commented Mar 14, 2024

Writing a flake8 plugin (https://flake8.pycqa.org/en/latest/plugin-development/index.html) would be one solution, or we could use a grep-based linting script.

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 a pull request may close this issue.

3 participants