-
Notifications
You must be signed in to change notification settings - Fork 19
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
Fix show_after error when sizeof fails to determine DataType size #421
Fix show_after error when sizeof fails to determine DataType size #421
Conversation
This error was preventing users from creating YAXArrays of Strings} and Unions that contain Strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for trying to tackle this, but I think, that it would be good to not have a try catch in the show method.
src/Cubes/Cubes.jl
Outdated
try | ||
println(io, " file size: ", formatbytes(cubesize(c))) | ||
catch e | ||
e isa ErrorException ? println(" could not determine DataType size.") : rethrow(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we rethrow the error, we will get a better message but still can't show the cube without an error. So I think that this should not be rethrowing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixcremer what's is blocking this?
src/Cubes/Cubes.jl
Outdated
@@ -510,7 +510,11 @@ function DD.show_after(io::IO, mime, c::YAXArray) | |||
blockwidth = get(io, :blockwidth, 0) | |||
DD.print_block_separator(io, "file size", blockwidth, blockwidth) | |||
println(io, " ") | |||
println(io, " file size: ", formatbytes(cubesize(c))) | |||
try |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the use of try catch in this.
Could we use Base.summarysize(first(c)) instead of sizeof(eltype(c)) in the definition of cubesize?
This should give the same result for plain types like Int and Float64 and gives an approximate size for other types like string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is, that we would introduce a getindex call which might be even more problematic than the use of try catch. So I am fine with that or with removing this info and only enabling that for certain eltypes where we know that sizeof is giving the right answer.
We could do that by putting the lines 511 to 513 together into a print_cubesize function which can then dispatch to the eltype of the cube and which can default to nothing to not print this information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @lazarusA
The problem is, that we would introduce a getindex call which might be even more problematic than the use of try catch. So I am fine with that or with removing this info and only enabling that for certain eltypes where we know that sizeof is giving the right answer. We could do that by putting the lines 511 to 513 together into a print_cubesize function which can then dispatch to the eltype of the cube and which can default to nothing to not print this information.
Thanks for the response and sorry for the delay.
I made a change checking for some specific eltypes for which the function sizeof
is known to behave well before printing the size of the datacube. Let me know if that approach is better or what you think I need to change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #421 +/- ##
==========================================
+ Coverage 68.30% 68.39% +0.08%
==========================================
Files 12 12
Lines 1773 1778 +5
==========================================
+ Hits 1211 1216 +5
Misses 562 562 ☔ View full report in Codecov by Sentry. |
This error is preventing users from creating YAXArrays of Strings} and Unions that contain Strings.
closes #410