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

Automatically change parameter and return float types #222

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jagoosw
Copy link
Collaborator

@jagoosw jagoosw commented Oct 14, 2024

This PR removes hard coded float types in function returns, and sets up most (not worth it to do the sugar kelp model because it has a billion parameters) model setup functions to automatically set the correct float type, or at least be able to set in a simple way, e.g.:

grid = RectilinearGrid(Float32; ...)
bgc = PISCES(; grid) # now has the correct FT
cc = CarbonChemistry(Float32)

cc: @ali-ramadhan @glwagner

Closes #218

@jagoosw jagoosw marked this pull request as ready for review October 15, 2024 18:00
@glwagner
Copy link
Collaborator

That looks nice, two comments:

  1. I prefer bgc = PISCES(grid). The interface is a bit split whether grid should be an arg vs kwarg but I think we have been gradually drifting towards grid as positional arg (the big change is for models, which will have to come later).

  2. There's a discussion about making it easier to change number type here: How can we help users change number type more easily? CliMA/Oceananigans.jl#3800

@glwagner
Copy link
Collaborator

(not worth it to do the sugar kelp model because it has a billion parameters)

I don't follow this statement --- what do you mean?

For situations with large numbers of parameters, you can put them in an Array (like TabluatedAlbedo for ClimaOcean):

https://github.com/CliMA/ClimaOcean.jl/blob/fc175f6c6b3e095bd460d10d323e4361006a5008/src/OceanSeaIceModels/CrossRealmFluxes/tabulated_albedo.jl#L90

Another possibility is to convert at runtime, eg with something like

const parameter = 1.23

f(a::FT) = a + convert(FT, parameter)

@jagoosw
Copy link
Collaborator Author

jagoosw commented Oct 24, 2024

Sorry I missed this I've been a bit busy.

I think that makes sense, there are a lot of instances here where (; grid) should probably be (grid; ) so I'll make a PR.

On the sugar kelp model, I didn't do it because I was being lazy but I'll change them before I merge the PR.

@glwagner
Copy link
Collaborator

On the sugar kelp model, I didn't do it because I was being lazy but I'll change them before I merge the PR.

You can also make a note in the source code and then open an issue about improvements if it doesn't make sense to prioritize immediately

Copy link
Collaborator

@ali-ramadhan ali-ramadhan left a comment

Choose a reason for hiding this comment

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

Also agree that this is nice and doesn't need to be perfect to be merged!

Maybe I'm misunderstanding the issue but with tons of parameters can't we loop over each property and convert it?

Something like this:

function Base.convert(::Type{SugarKelp{FT_new, TL}}, x::SugarKelp{FT_old, TL}) where {FT_new, FT_old, TL}
    fields = NamedTuple{fieldnames(typeof(x))}(
        field === :temperature_limit ? getfield(x, field) : convert(FT_new, getfield(x, field))
        for field in fieldnames(typeof(x))
    )

    return SugarKelp{FT_new, TL}(; fields...)
end

@jagoosw
Copy link
Collaborator Author

jagoosw commented Oct 29, 2024

Maybe I'm misunderstanding the issue but with tons of parameters can't we loop over each property and convert it?

Something like this:

function Base.convert(::Type{SugarKelp{FT_new, TL}}, x::SugarKelp{FT_old, TL}) where {FT_new, FT_old, TL}
    fields = NamedTuple{fieldnames(typeof(x))}(
        field === :temperature_limit ? getfield(x, field) : convert(FT_new, getfield(x, field))
        for field in fieldnames(typeof(x))
    )

    return SugarKelp{FT_new, TL}(; fields...)
end

Oh yes that's a good idea, I hadn't thought of that.

Do you think we would then need to put in the Biogeochemistry constructor a convert(FT, particles) (which would mean we always have to have convert defined, or do you think it would be fine to leave this to the user like:

particles = SugarKelp(...)

particles = convert(FT, particles)

biogeochemistry = Biogeochemistry(...; particles)

@glwagner
Copy link
Collaborator

do you think it would be fine to leave this to the user like:

You want to throw an error if the float type is wrong

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.

Hard-coded Float64 values
3 participants