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.

Sign up for GitHub

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

Jump to bottom

Fix show_after error when sizeof fails to determine DataType size #421

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+5 −3
Open

Fix show_after error when sizeof fails to determine DataType size #421

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bfe7df
Fix show_after error when sizeof fails to determine DataType size
Zapiano Aug 9, 2024
51d5f82
Remove try catch and only print cube size for a few selected types
Zapiano Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/Cubes/Cubes.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Author

@Zapiano Zapiano Sep 5, 2024 •
edited
Loading

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.

println(io, " file size: ", formatbytes(cubesize(c)))
catch e
e isa ErrorException ? println(" could not determine DataType size.") : rethrow(e)
Copy link
Member

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.

Copy link
Collaborator

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?

end
DD.print_block_close(io, blockwidth)
# And if you want the array data to print:
# ndims(c) > 0 && println(io)
Expand Down