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 all hardcoded Float64 from the Advection module #4053

Merged
merged 8 commits into from
Jan 23, 2025

Conversation

simone-silvestri
Copy link
Collaborator

@simone-silvestri simone-silvestri commented Jan 20, 2025

and also correct some Float64 that slipped in the SplitExplicitFreeSurface. This is some preparatory work to allow more flexible user type and eventually fully support Metal.

Note that I have tested Metal on an additional branch that implements these changes + some (quite easy actually) support for Metal architectures https://github.com/CliMA/Oceananigans.jl/tree/ss/fix-metal (as well as some other tangential stuff I was trying which is not relevant) and this example seem to run quite nicely with WENO, so after this PR is merged, the route for Metal should be

  • enable easy Float32 support
  • implement a MetalGPU (alias for a possible GPU{<:Metal.MetalBackend}) architecture

U = deepcopy(free_surface.barotropic_velocities.U)
V = deepcopy(free_surface.barotropic_velocities.V)
U = similar(free_surface.barotropic_velocities.U)
V = similar(free_surface.barotropic_velocities.V)
Copy link
Member

Choose a reason for hiding this comment

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

This is unrelated right? The difference is that “similar” doesn’t copy boundary conditions.

Copy link
Collaborator Author

@simone-silvestri simone-silvestri Jan 22, 2025

Choose a reason for hiding this comment

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

Right, this was a leftover from the Metal branch (that I stripped down for this PR). Apparently, there is a bug with deepcopy and Metal OffsetArrays:

julia> a = MtlArray(zeros(Float32, 4));

julia> a = OffsetArray(a, -1);

julia> b = deepcopy(a);

julia> fill!(b, 1);

julia> a
4-element OffsetArray(::MtlVector{Float32, Metal.PrivateStorage}, 0:3) with eltype Float32 with indices 0:3:
 1.0
 1.0
 1.0
 1.0

I wanted to raise an issue in OffsetArrays.jl but I haven't done it yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

with similar, on the other hand, there is no issue:

julia> a = MtlArray(zeros(Float32, 4));

julia> a = OffsetArray(a, -1);

julia> b = similar(a);

julia> a
4-element OffsetArray(::MtlVector{Float32, Metal.PrivateStorage}, 0:3) with eltype Float32 with indices 0:3:
 0.0
 0.0
 0.0
 0.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that this is not an issue with standard metal arrays

julia> a = MtlArray(zeros(Float32, 4));

julia> b = deepcopy(a);

julia> fill!(b, 1);

julia> a
4-element MtlVector{Float32, Metal.PrivateStorage}:
 0.0
 0.0
 0.0
 0.0

Copy link
Member

Choose a reason for hiding this comment

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

weird

@simone-silvestri simone-silvestri merged commit 373647f into main Jan 23, 2025
48 checks passed
@simone-silvestri simone-silvestri deleted the ss/fix-float-32 branch January 23, 2025 06:51
@@ -132,6 +132,10 @@ function __init__()
end
end

# Supported types for the Advection module.
Copy link
Member

@glwagner glwagner Jan 23, 2025

Choose a reason for hiding this comment

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

This comment should say something like:

# List of fully-supported floating point types where applicable.
# Currently used in the Advection module for WENO advection schemes.

This comment communicates that the type is designed to be more general than just for advection, and that the comment should be changed if future development occurs.

Currently, the comment is misleading because one can interpret it to mean that a different variable should be used to specify floating point types for other modules.

@@ -132,6 +132,10 @@ function __init__()
end
end

# Supported types for the Advection module.
# To support new floating point types, add them here
const supported_float_types = [Float32, Float64]
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be a tuple rather than array. Otherwise users can change it eg

push!(Oceananigans.supported_float_types, Float16)

but of course this does nothing.

@simone-silvestri
Copy link
Collaborator Author

Sounds good, I ll do the changes

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.

2 participants