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

Hard-coded Float64 values #218

Closed
glwagner opened this issue Sep 26, 2024 · 4 comments · Fixed by #222
Closed

Hard-coded Float64 values #218

glwagner opened this issue Sep 26, 2024 · 4 comments · Fixed by #222

Comments

@glwagner
Copy link
Collaborator

glwagner commented Sep 26, 2024

I suspect there will be people cropping up (👀 @ali-ramadhan) want to use Float32. This will thwart them:

@inline redfield(::Union{Val{:P}, Val{:Z}, Val{:D}}, bgc::NPZD) = 6.56

All that's needed is

@inline redfield(::Union{Val{:P}, Val{:Z}, Val{:D}}, bgc::NPZD{FT}) where FT = convert(FT, 6.56)

The key is to never write decimals in source code without convert.

@johnryantaylor
Copy link
Collaborator

Thanks for the tip @glwagner!

@ali-ramadhan
Copy link
Collaborator

Not sure if nitpicking but would

@inline redfield(::Union{Val{:P}, Val{:Z}, Val{:D}}, bgc::NPZD{FT}) where FT = FT(6.56)

be better than

@inline redfield(::Union{Val{:P}, Val{:Z}, Val{:D}}, bgc::NPZD{FT}) where FT = convert(FT, 6.56)

to avoid extra layer(s) of method inference during compilation?

@glwagner
Copy link
Collaborator Author

glwagner commented Oct 7, 2024

Well, I thought they were the same!

In Oceananigans source code we have been trying to migrate to convert(FT, x) simply because we felt it was more semantically obvious than merely FT(x) which is easier to forget exactly what it does first thing in the morning or at the end of a long day (well, it converts x to FT of course).

@ali-ramadhan
Copy link
Collaborator

That makes sense! For floats it's a super simple method inference. Maybe for more complex types there's more extra inference. But agree it's good to be more semantically obvious!

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