Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
tagging @jsignell to review |
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
We should probably be recommending that people use virtualizarr even if they are using it to write kerchunk reference files.
Reply via ReviewNB
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
Maybe just don't describe the functions here since it is bound to get out of sync with the functions and docstrings.
Reply via ReviewNB
There was a problem hiding this comment.
Done removed the function list from the markdown; docstrings on the helpers are the source of truth.
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
Line #53. return xr.open_datatree(fs.open(url, block_size=4 * 2**20, size=remote_size(url)),
Don't you need cache_type="blockcache" in here to get the block_size any impact?
Reply via ReviewNB
There was a problem hiding this comment.
Doneadded cache_type="blockcache" so block_size actually batches the range reads.
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Done, making it print
There was a problem hiding this comment.
2 requests doesn't seem like that many. The text makes it seem like the metadata is sprinkled all over which would imply that a ton of requests would be needed to construct the lazy data tree.
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
There is also a much smaller chunk size that is recommended for visualization, but not sure that applies to this particular dataset.
I don't understand what this means: "A codec column full of None is the quantitative version of the feedback that compression belongs inside the netCDF.". Are you just trying to say: "A codec column full of None means there is not compression within the NetCDF"?
Reply via ReviewNB
There was a problem hiding this comment.
Yes, reworded to exactly that
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
This cell should have a narrative description. What you have here as "logical size" is really the Python in-memory size. I am not sure 100% sure those are the same thing.
Reply via ReviewNB
There was a problem hiding this comment.
Done added a short narrative and renamed it to in-memory size (sum of nbytes), noting that isn’t always the same as HDF5 logical size.
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
the demo was off. Fixed to an HDF5-style byte shuffle on one real on-disk chunk
| @@ -0,0 +1,1938 @@ | |||
| { | |||
There was a problem hiding this comment.
This part feels random and tacked on. Does it just need a better description or can it reuse some of the helper functions?
Also where is the kerchunk part?
Reply via ReviewNB
There was a problem hiding this comment.
Done moved it under the METimage walkthrough,added the VirtualiZarr → kerchunk section there.
jsignell
left a comment
jsignell
left a comment
There was a problem hiding this comment.
Broadly it would be helpful when reviewing to know more about the intended audience for the notebook and how much of the notebook was generated by AI.
| @@ -0,0 +1,2668 @@ | |||
| { | |||
There was a problem hiding this comment.
It's weird that this got slower than in the original notebook. Maybe since the chunks are so small "blockcache" should not be recommended after all.
Reply via ReviewNB
| @@ -0,0 +1,2668 @@ | |||
| { | |||
There was a problem hiding this comment.
the on-disk file may still be smaller than the sum of nbytes for other reasons, such as sparse fill or unallocated extents, but HDF5 filters are not compressing the arrays)
This is the wrong comparison. You should be comparing dtype.itemsize * nelements with the file size, not nbytes
Reply via ReviewNB
| @@ -0,0 +1,2668 @@ | |||
| { | |||
There was a problem hiding this comment.
I don't think this is right. If you look at the function you'll see that nbytes is the size of the in-memory representation and chunk_MB is the idealized uncompressed size of the data on disk (dtype.itemsize * nelements).
Reply via ReviewNB
| @@ -0,0 +1,2668 @@ | |||
| { | |||
There was a problem hiding this comment.
Why did you change this from the 512x512 window? And why do you think the compression ratios got sooo much better.
Reply via ReviewNB
There was a problem hiding this comment.
For the compression test, is it better to use a normal mid-image 512×512 sample, or to compress one real on-disk chunk and note when that chunk is mostly fill/empty (which can make the ratios look unrealistically high)?
| @@ -0,0 +1,2668 @@ | |||
| { | |||
There was a problem hiding this comment.
You can probably simplify this by using engine="kerchunk" . Also any ideas why the json is larger than in the original? Is it possible you are appending to an existing file or something?
Also what are you trying to show? It looks like it takes the same amount of time to access a slice of data as it did in the naive version which makes sense, but it maybe needs a bit of a "why we should care about virtual zarr stores".
Reply via ReviewNB
| @@ -0,0 +1,2668 @@ | |||
| { | |||
There was a problem hiding this comment.
|
@jsignell I dont have more information about the intended audience, just reviewing the comments here. @wildintellect @omshinde any thoughts on who the end users are? |
4 participants
Adds notebooks for accessing and assessing EUMETSAT data products for cloud-native readiness.