Adding WriterBuilder::begin_2d

[Yomguithereal]

Hi @ExpHP, here is the PR adding WriterBuilder::begin_2d as discussed in #78.

The only thing which remains unclear to me is whether shape should be written the other way around when using FORTRAN order, but I think not.

I have used record_size as per your suggestion.

Have a great day,

[ExpHP]

Sounds exciting! I've got a bit of a headache right now but I will take a look this week

[ExpHP]

Taking a look right now

[ExpHP]

I really appreciate the division into two commits. The first commit deserves a chef's kiss.

It took me a surprisingly long time to come up with compelling evidence for the transpose argument without needing to do some jank like "np.save and compare the written bytes after slicing the header away". Surprisingly, both bytes(array.data) and .tobytes(order="K") end up allocating a second temporary buffer to rewrite the data into C contiguous order, producing different results from the payload written to an npy file. (The order="K" one is a still-open bug that was reported in 2020...).

I ended up needing to go as far as having 3 arrays all alias the exact same buffer in memory. While I can't directly show that they share the same bytes, I can show that they share the same address in memory! 😂

[ExpHP]

• Note to self that the definition of FILLER_FOR_UNKNOWN_SIZE is not consistent with its usage, as it does not include the closing parens.

[Yomguithereal]

Whay is FILLER_FOR_UNKNOWN_SIZE is 19 by the way? Shouldn't it be 20, e.g. the maximum number of digits a u64 can have (without even considering your note above considering the closing parens)?

[ExpHP]

I was wondering that myself. I don't recall how I chose that number. I should add a unit test for begin_1d that has item size zero and len u64::MAX.

[Yomguithereal]

I was wondering that myself. I don't recall how I chose that number. I should add a unit test for begin_1d that has item size zero and len u64::MAX.

I can investigate and add a unit test for that while I am amending the PR if you want.

[ExpHP]

Sure, I'd appreciate it. Now that I think about it I probably have one already (can't check on my phone), but it might be succeeding too easily due to the extra space for alignment... that could make it really hard to make a test that reliably detects a regression and might not be worth it.

As an alternative, we could maybe just write the max u64 value as a placeholder instead of a const str. That should be easy to reason about, if we pull out a function for writing a shape for a given len that gets used both at begin and finish. (so that there can be no mismatch in length of the preceeding or trailing characters)

[Yomguithereal]

As an alternative, we could maybe just write the max u64 value as a placeholder instead of a const str.

I think using your ****... placeholder is safer. I don't like the idea of using the max u64 there (apart for testing purposes). Else some parsers/readers might pre-allocate the data to a max u64 and crash before even realizing the file is lying to them. At least with the * it will crash before :)

[Yomguithereal]

@ExpHP I have amended the PR to:

1. rename record_size to record_len
2. Bump FILLER_FOR_UNKNOWN_SIZE to 20 (max number of digits in a u64)
3. added correct Order::Fortran support (and removed shameful bad .unwrap() calls in finish_)

I could not find what you think is problematic currently regarding FILLER_FOR_UNKNOWN_SIZE. It seems to do its job correctly and is not supposed to take the ",), }" part into account in its size. Maybe it's just a question of amending the comment above its definition?

[ExpHP]

At least with the * it will crash before :)

Good argument, I agree.

Maybe it's just a question of amending the comment above its definition?

Eh, it's just the fact that the total number of characters written depends on the number of spaces and presence/lack of trailing commas, and that it's not immediately obvious that these factors match between the two places.

I will merge this as is, play around with it a bit and prep for release.

[ExpHP]

also going to audit for any other unwraps on IO and look into how this one slipped through ...oh, it was yours. Gotcha. Still gonna do an audit tho