Change default output array type for JLD2OutputWriter to Array{Float64}
#2890
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t64}
While `Array{Float32}` is neat because user output is smaller, it's a bad default because it often leads to confusing results, like the fact that outputing `Field(Integral(c))` is different (possibly by `eps(Float32)`) than computing `Integral(c_output)` in post processing.
Thus @xkykai and I propose to make the default `Array{Float64}`. We could regard outputting with `Array{Float32}` as an "optimization" which is premature as a default.
glwagner
changed the title
JLD2OutputWriter to Array{Floa…JLD2OutputWriter to Array{Float64}`
glwagner
changed the title
JLD2OutputWriter to Array{Float64}`JLD2OutputWriter to Array{Float64}
|
cc @navidcy we also ran into this problem with ParameterEstimocean once |
I remember… took days to figure out what was happening |
navidcy
approved these changes
Feb 1, 2023
|
Only for JLD2OutputeWriter? |
Good point. |
navidcy
approved these changes
Feb 3, 2023
navidcy
reviewed
Feb 3, 2023
test/test_netcdf_output_writer.jl
Outdated
Comment on lines
199
to
204
| @test eltype(ds2["xC"]) == Float64 | ||
| @test eltype(ds2["xF"]) == Float64 | ||
| @test eltype(ds2["yC"]) == Float64 | ||
| @test eltype(ds2["yF"]) == Float64 | ||
| @test eltype(ds2["zC"]) == Float64 | ||
| @test eltype(ds2["zF"]) == Float64 |
There was a problem hiding this comment.
ds2 is nc_sliced_writer which was constructed with explicit array_type = Array{Float32}
navidcy
reviewed
Feb 3, 2023
|
(fyi I stopped the tests because everything was clogged... all tests in PRs have been running for 6-7 hrs... so perhaps restart when there is less load) |
|
merge? |
|
Let's bump |
|
why 0.79.3? we are only up to 0.79.1... am I missing something? |
you;re right |
|
Merge? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While
Array{Float32}is neat because user output is smaller, it's a bad default because it often leads to confusing results, like the fact that outputingField(Integral(c))is different (possibly byeps(Float32)) than computingIntegral(c_output)in post processing whenField(Integral(c))is close to 0.Thus @xkykai and I propose to make the default
Array{Float64}. We could regard outputting withArray{Float32}as an "optimization" which is premature as a default.