Opened 3 years ago

Closed 3 years ago

#23411 closed defect (fixed)

Fix pickling of matrix_gfpn_dense

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-8.1
Component: packages: optional Keywords:
Cc: Merged in:
Authors: Simon King Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: ba72aba (Commits) Commit: ba72abac8a53b3d292df570d31709ab5c2f57157
Dependencies: Stopgaps:

Description

There currently are two problems related with pickling of Matrix_gfpn_dense:

  • When a string is provided as argument to __init__, it is interpreted as the name of a file from which data are read by some MeatAxe command. If the file doesn't exist, it was intended to have an uninitialised matrix (to be properly initialised later). But currently it crashes, which needs to be fixed.
  • The number of bytes used holding the data of a matrix row is machine independent. However, the number of bytes actually used to store the row always is a multiple of sizeof(long), which is machine dependent (at least in theory). Pickling should of course only involve the machine independent memory chunks, but currently it involves the machine dependent chunks.

Change History (70)

comment:1 Changed 3 years ago by SimonKing

  • Branch set to u/SimonKing/fix_pickling_of_matrix_gfpn_dense

comment:2 Changed 3 years ago by SimonKing

  • Authors set to Simon King
  • Commit set to 4e85fca604650a1f29ff2ae13aa1263737685891
  • Status changed from new to needs_review

New commits:

4960a81Fix reading mtx-matrix from non-existent file
4e85fcaMake pickling of matrix_gfpn_dense machine independent

comment:3 Changed 3 years ago by SimonKing

  • Status changed from needs_review to needs_work

Sorry, one commit is missing

comment:4 Changed 3 years ago by git

  • Commit changed from 4e85fca604650a1f29ff2ae13aa1263737685891 to f9d878abdf0c0742ffaeefe2176c5217521b3dd7

Branch pushed to git repo; I updated commit sha1. New commits:

f9d878aAdd test for reading from non-existing file; add cimports

comment:5 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

Now it builds and tests are passing...

comment:6 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. As I mentioned on some other ticket, use bytes instead of str for pickling.
  1. If you claim that this makes pickling machine-independent, it would be good to actually test that. You can hardcode a pickle in a doctest (essentially, the output of dumps()) and then test that.

comment:7 in reply to: ↑ 6 ; follow-ups: Changed 3 years ago by SimonKing

Replying to jdemeyer:

  1. As I mentioned on some other ticket, use bytes instead of str for pickling.

You also said I should split my original patchbomb into smaller parts. That's why I wanted to leave the transition from str to bytes for a ticket dedicated to cleaning code.

  1. If you claim that this makes pickling machine-independent, it would be good to actually test that. You can hardcode a pickle in a doctest (essentially, the output of dumps()) and then test that.

OK. Just to be on the safe side: You do not say that I should imitate a pickle string that results from a machine where, say, sizeof(long)==3? Cause otherwise we would never see the deprecation warning.

comment:8 in reply to: ↑ 7 Changed 3 years ago by SimonKing

Replying to SimonKing:

You also said I should split my original patchbomb into smaller parts. That's why I wanted to leave the transition from str to bytes for a ticket dedicated to cleaning code.

PS: Adding such patch would also imply to modify the lines where I import the PyString... functions, which means that there will be further conflicts with #21437.

comment:9 Changed 3 years ago by git

  • Commit changed from f9d878abdf0c0742ffaeefe2176c5217521b3dd7 to e477948bb0a393997082fffea411e1724517ecaa

Branch pushed to git repo; I updated commit sha1. New commits:

e477948Test meataxe unpickling being machine independent

comment:10 Changed 3 years ago by SimonKing

In the latest commit, I add a test that should demonstrate machine independence of pickling (provided that the patchbots provide a whole lot of different architectures, including little and big endian and different sizes of long). I also add a test that triggers the deprecation warning.

I am not really happy about doing part of the code cleanup here. Would you accept to do all cleanup on a separate ticket. If you do, then please consider this ticket as "needs review".

comment:11 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

Setting it to "needs review" in order to allow the patchbots to test the ticket.

comment:12 Changed 3 years ago by git

  • Commit changed from e477948bb0a393997082fffea411e1724517ecaa to 01e2c0dcbcf05ef8dbe4b1f83b8334873ebd6f5a

Branch pushed to git repo; I updated commit sha1. New commits:

01e2c0dMark new meataxe test 'optional'

comment:13 Changed 3 years ago by SimonKing

Question: Will the buildbot automatically test with and without meataxe being installed? If not: How can I advice the buildbot to do so?

comment:14 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

  1. As I mentioned on some other ticket, use bytes instead of str for pickling.

You also said I should split my original patchbomb into smaller parts. That's why I wanted to leave the transition from str to bytes for a ticket dedicated to cleaning code.

It's just a bit strange that you write "unclean" code in one ticket and then at the same time have another ticket to clean it up.

But that's not a big deal since the old code also uses strings.

comment:15 Changed 3 years ago by jdemeyer

Also in some other ticket, I made the comment that you should use floor division (//) if that's what you want.

comment:16 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Please do not use the for ... from ... syntax. Cython understands range().

comment:17 Changed 3 years ago by jdemeyer

When unpickling, it seems that there is no protecting against reading out of bounds. The case where x is too short should be handled gracefully. The code should be more like:

if len(Data) == expected_size:
    # Handle new-style pickle
elif len(Data) == expected_size_for_old_pickle:
    # Handle deprecated pickle
else:
    raise ValueError(f"expected a pickle with {expected_size} bytes, got {len(Data)} instead")

EDIT: note the swapped conditions. It is possible that expected_size == expected_size_for_old_pickle.

And a small detail: you write twice x = PyString_AsString(Data) while once (outside the branch) should be sufficient.

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:18 in reply to: ↑ 14 Changed 3 years ago by SimonKing

Replying to jdemeyer:

It's just a bit strange that you write "unclean" code in one ticket and then at the same time have another ticket to clean it up.

But that's not a big deal since the old code also uses strings.

Right. And you spotted further problems with the code added by this ticket.

So, I guess it is better to clean the new code, although it will imply manual merging in other tickets.

comment:19 follow-up: Changed 3 years ago by jdemeyer

If I were you, I would first focus on getting these first tickets accepted, otherwise you will be forever merging and rebasing :-)

comment:20 Changed 3 years ago by git

  • Commit changed from 01e2c0dcbcf05ef8dbe4b1f83b8334873ebd6f5a to d1748351fe1ab0201647ce98b83c29188534a2ea

Branch pushed to git repo; I updated commit sha1. New commits:

d174835Check bounds when unpickling meataxe matrices

comment:21 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:22 in reply to: ↑ 19 Changed 3 years ago by SimonKing

Replying to jdemeyer:

If I were you, I would first focus on getting these first tickets accepted, otherwise you will be forever merging and rebasing :-)

Hooray, it merges cleanly into #21437...

I guess I'll wait a while till I move on to #23400.

comment:23 Changed 3 years ago by SimonKing

PS: Perhaps it would have been better to *first* clean the existing code, and then do the bug fixes and additions. But I suppose I shouldn't change the order of tickets again, or should I?

comment:24 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This is a potential division by zero:

cdef size_t pickled_rowsize = lenData//nr

It would be good to add a doctest for the case nr == 0.

I think the best solution is to replace the condition if Data by if nr.

comment:25 Changed 3 years ago by git

  • Commit changed from d1748351fe1ab0201647ce98b83c29188534a2ea to 8873330db79f5aa72de43f9be0e7babba07b05dd

Branch pushed to git repo; I updated commit sha1. New commits:

8873330Mark further meataxe tests optional; fix potential zero division

comment:26 Changed 3 years ago by git

  • Commit changed from 8873330db79f5aa72de43f9be0e7babba07b05dd to 7a1ed5adf98cdbfbc4300205faa584df83728766

Branch pushed to git repo; I updated commit sha1. New commits:

7a1ed5aMeataxe unpickling: Further consistency checks

comment:27 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:28 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Assertions are meant to guard against bugs, not against invalid input. Your assert should be a raise ValueError.

comment:29 follow-up: Changed 3 years ago by jdemeyer

It would be good to add doctests for correct unpickling of matrices with zero rows or columns.

comment:30 in reply to: ↑ 28 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Assertions are meant to guard against bugs, not against invalid input. Your assert should be a raise ValueError.

OK. And I guess since I am touching some sub-optimal code here (using PyString...), it'd be better to clean this here, not in #23400

comment:31 in reply to: ↑ 29 ; follow-up: Changed 3 years ago by SimonKing

Replying to jdemeyer:

It would be good to add doctests for correct unpickling of matrices with zero rows or columns.

Do you mean a test with hard-coded pickle string? Or a test in the form loads(dumps(M))==M?

comment:32 in reply to: ↑ 31 Changed 3 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

It would be good to add doctests for correct unpickling of matrices with zero rows or columns.

Do you mean a test with hard-coded pickle string? Or a test in the form loads(dumps(M))==M?

I meant the latter. Although an explicit call to mtx_unpickle with empty Data would be good also.

comment:33 in reply to: ↑ description ; follow-ups: Changed 3 years ago by jdemeyer

Replying to SimonKing:

  • When a string is provided as argument to __init__, it is interpreted as the name of a file from which data are read by some MeatAxe command. If the file doesn't exist, it was intended to have an uninitialised matrix (to be properly initialised later). But currently it crashes, which needs to be fixed.

I don't like your fix for this for several reasons:

  1. I can't actually reproduce the issue. Without this patch, the doctest that you added passes. I think this is because the memory for Python objects is automatically initialized with all zeros.
  1. If the result of __cinit__ is really invalid in some way, then it means that there is something wrong with __cinit__. Most importantly, __cinit__ should already return an object which doesn't cause segfaults.
  1. A non-existing file should be an error, not something to be silently ignored.
  1. Do you really need to open the file in Python and then close it and then let MeatAxe open it again? At a minimum, you should document in a comment why you are doing it this way.

From reading the code, I think the best solution would be to remove __cinit__ completely and handle all initialization in __init__.

comment:34 Changed 3 years ago by git

  • Commit changed from 7a1ed5adf98cdbfbc4300205faa584df83728766 to f274beb66bfee75ff975b72795ffbafb5cdd178b

Branch pushed to git repo; I updated commit sha1. New commits:

f274bebMeataxe unpickling: Further corner cases

comment:35 in reply to: ↑ 33 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

  • When a string is provided as argument to __init__, it is interpreted as the name of a file from which data are read by some MeatAxe command. If the file doesn't exist, it was intended to have an uninitialised matrix (to be properly initialised later). But currently it crashes, which needs to be fixed.

I don't like your fix for this for several reasons:

  1. I can't actually reproduce the issue.

Strange. has there been a recent change in Cython? If I recall correctly, reading from a non-existing file left the matrix in an undefined state. In particular, it was possible that the self.Data pointer was *not* NULL.

  1. A non-existing file should be an error, not something to be silently ignored.

OK.

  1. Do you really need to open the file in Python and then close it and then let MeatAxe open it again? At a minimum, you should document in a comment why you are doing it this way.

From reading the code, I think the best solution would be to remove __cinit__ completely and handle all initialization in __init__.

If it is really the case that I can rely on self.Data being initialised to NULL, then I guess removing __cinit__ is fine.

comment:36 Changed 3 years ago by git

  • Commit changed from f274beb66bfee75ff975b72795ffbafb5cdd178b to 6586e97ae1f092eb3a8d2ad9d0da4c7728a87cdc

Branch pushed to git repo; I updated commit sha1. New commits:

6586e97Sanitise meataxe matrix initialisation

comment:37 in reply to: ↑ 33 Changed 3 years ago by SimonKing

Replying to jdemeyer:

Replying to SimonKing:

  1. If the result of __cinit__ is really invalid in some way, then it means that there is something wrong with __cinit__. Most importantly, __cinit__ should already return an object which doesn't cause segfaults.

That is the case, provided that self.Data is correctly initialised to NULL by cython.

  1. A non-existing file should be an error, not something to be silently ignored.

Done. Now, an error raised by MeatAxe is propagated.

  1. Do you really need to open the file in Python and then close it and then let MeatAxe open it again?

No, one can simply catch the resulting error, if one likes...

From reading the code, I think the best solution would be to remove __cinit__ completely and handle all initialization in __init__.

Done. And the tests pass :)

comment:38 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:39 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

There is something wrong with the indentation here:

        5. Creating a matrix from a file in MeatAxe format. If the file doesn't exist,
            an error raised by the MeatAxe library is propagated::

            sage: Matrix_gfpn_dense('foobarNONEXISTING_FILE')       # optional: meataxe
            Traceback (most recent call last):
            ...
            SystemError: .../foobarNONEXISTING_FILE: No such file or directory in file os.c (line 254)

Correct indentation would be

        5.  Creating a matrix from a file in MeatAxe format. If the file doesn't exist,
            an error raised by the MeatAxe library is propagated::

                sage: Matrix_gfpn_dense('foobarNONEXISTING_FILE')       # optional: meataxe
                Traceback (most recent call last):
                ...
                SystemError: .../foobarNONEXISTING_FILE: No such file or directory in file os.c (line 254)

(note the double space after 5. to make allow 4 spaces of indentation on the next line)

comment:40 Changed 3 years ago by git

  • Commit changed from 6586e97ae1f092eb3a8d2ad9d0da4c7728a87cdc to 77bc58b647af83ca457798bd57e7c57b57a1a6a2

Branch pushed to git repo; I updated commit sha1. New commits:

77bc58bFix indentation in meataxe matrix __init__ doc

comment:41 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:42 follow-up: Changed 3 years ago by jdemeyer

What is the purpose of

            if not parent:
                self._cache == {}

It seems that you want to allow Matrix_gfpn_dense("") to return an uninitialized matrix but I wonder why. If you really want that for some reason, it would be better to replace if parent is None above by if not parent.

comment:43 Changed 3 years ago by jdemeyer

Except for my last comment, this ticket is fine for me.

comment:44 in reply to: ↑ 42 ; follow-up: Changed 3 years ago by SimonKing

Replying to jdemeyer:

What is the purpose of

            if not parent:
                self._cache == {}

It seems that you want to allow Matrix_gfpn_dense("") to return an uninitialized matrix but I wonder why. If you really want that for some reason, it would be better to replace if parent is None above by if not parent.

I do use uninitialised meataxe matrices in some application (of course in my group cohomology package), namely when I want to create a Matrix_gfpn_dense from an existing Matrix_t*. But your suggestion makes perfect sense.

comment:45 Changed 3 years ago by git

  • Commit changed from 77bc58b647af83ca457798bd57e7c57b57a1a6a2 to 5106ebb42dcc69206b33d15c4a96f8a14fbc2f8c

Branch pushed to git repo; I updated commit sha1. New commits:

5106ebbSimplified code path for uninitialised meataxe matrices

comment:46 Changed 3 years ago by SimonKing

Done!

comment:47 in reply to: ↑ 44 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

I do use uninitialised meataxe matrices in some application

I would argue that the proper way to get an "uninitialised meataxe matrix" is precisely to do Matrix_gfpn_dense.__new__(Matrix_gfpn_dense).

This idiom of using __new__ and then manually filling in a data structure is used a lot in Sage. For example, in the numerator method for rational numbers:

    def numerator(self):
        cdef Integer n = Integer.__new__(Integer)
        n.set_from_mpz(mpq_numref(self.value))
        return n

comment:48 in reply to: ↑ 47 Changed 3 years ago by SimonKing

  • Status changed from needs_review to needs_work

Replying to jdemeyer:

I would argue that the proper way to get an "uninitialised meataxe matrix" is precisely to do Matrix_gfpn_dense.__new__(Matrix_gfpn_dense).

Actually I just checked: I am using the .__new__ thing in some part of my cohomology code, and in some other parts I'd really better do it as well. I suppose it should be removed.

comment:49 Changed 3 years ago by SimonKing

PS: Also note the statement in the docstring of __init__, where I say that the case parent = None is used for unpickling --- which is not true, as I am using __new__ there as well!

comment:50 Changed 3 years ago by git

  • Commit changed from 5106ebb42dcc69206b33d15c4a96f8a14fbc2f8c to 3d94133f7514a2d5f3887aabeb2a6068e981a272

Branch pushed to git repo; I updated commit sha1. New commits:

3d94133Remove codepath for creating uninitialised meataxe matrix

comment:51 follow-up: Changed 3 years ago by jdemeyer

Avoid six in Cython files if possible! Please revert to basestring.

comment:52 in reply to: ↑ 51 ; follow-ups: Changed 3 years ago by SimonKing

Replying to jdemeyer:

Avoid six in Cython files if possible! Please revert to basestring.

Really? I thought this was needed to make things Python3 compliant. OK, I'll revert it.

Something else: If the MeatAxe function MatLoad gets an empty file name, it apparently just returns the NULL pointer, but doesn't raise an error. What shall I do? Shall I change Matrix_t *MatLoad(char *fn) except NULL into Matrix_t *MatLoad(char *fn) except? NULL (or what's the syntax if NULL can also be returned without an error)?

comment:53 in reply to: ↑ 52 Changed 3 years ago by jdemeyer

Replying to SimonKing:

I thought this was needed to make things Python3 compliant.

Yes, six is needed to make things Python 3 compliant.

However, Cython is a language by itself. Cython is not Python 2 and it's not Python 3. Cython tries to be compatible with Python 2 and Python 3. This makes porting Cython code to Python 3 much easier than porting Python code.

Of course, some Cython code is really just Python code and that code can depend on the Python version. And there are some incompatibilities that Cython cannot fix: support for __cmp__ is gone in Python 3, there is nothing that Cython can do about that.

comment:54 in reply to: ↑ 52 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

Avoid six in Cython files if possible! Please revert to basestring.

Really? I thought this was needed to make things Python3 compliant. OK, I'll revert it.

Something else: If the MeatAxe function MatLoad gets an empty file name, it apparently just returns the NULL pointer, but doesn't raise an error. What shall I do? Shall I change Matrix_t *MatLoad(char *fn) except NULL into Matrix_t *MatLoad(char *fn) except? NULL (or what's the syntax if NULL can also be returned without an error)?

Yes, except? NULL is fine. I would still raise a Python exception in this case though.

comment:55 in reply to: ↑ 54 ; follow-up: Changed 3 years ago by SimonKing

Replying to jdemeyer:

Yes, except? NULL is fine. I would still raise a Python exception in this case though.

If you want an error being raised on Matrix_gfpn_dense(''), then I need to re-introduce the test if not parent (but then raise an error). Is that OK for you?

In any case, changing the function header of MatLoad should be done.

comment:56 in reply to: ↑ 55 ; follow-up: Changed 3 years ago by jdemeyer

Replying to SimonKing:

Replying to jdemeyer:

Yes, except? NULL is fine. I would still raise a Python exception in this case though.

If you want an error being raised on Matrix_gfpn_dense(''), then I need to re-introduce the test if not parent (but then raise an error). Is that OK for you?

But you say that MeatAxe returns NULL without exception in this case. So it suffices to check the result of MatLoad().

comment:57 in reply to: ↑ 56 Changed 3 years ago by SimonKing

Replying to jdemeyer:

But you say that MeatAxe returns NULL without exception in this case. So it suffices to check the result of MatLoad().

From my perspective, one could simply return an empty 0x0-matrix over an unspecified field in that special case. But if you prefer an error being raised (ValueError("Received NULL pointer from initialisation of MeatAxe matrix"), perhaps?), I can do that.

comment:58 Changed 3 years ago by jdemeyer

I consider it a bug that MatLoad("") returns NULL without an error. So we are just working around a MeatAxe bug here...

comment:59 Changed 3 years ago by git

  • Commit changed from 3d94133f7514a2d5f3887aabeb2a6068e981a272 to 982755f103b4d1225051c060f93e9a4069deb87a

Branch pushed to git repo; I updated commit sha1. New commits:

564104eDon't use six.string_types, but basestring
982755fWorkaround: MeatAxe's MatLoad returns NULL on empty filename without setting error

comment:60 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:61 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

As expected, this "machine dependent" doctest may fail depending on the machine:

        sage: t = 'Uq\x82\x00\x00\x00\x00\x00\xa7\x8bh\x00\x00\x00\x00\x00'
        sage: len(t)
        16
        sage: N == mtx_unpickle(MS, 2, 5, t, True)           # optional: meataxe

comment:62 follow-up: Changed 3 years ago by jdemeyer

On a 32-bit system, this gives:

sage -t --long --warn-long 87.3 src/sage/matrix/matrix_gfpn_dense.pyx
**********************************************************************
File "src/sage/matrix/matrix_gfpn_dense.pyx", line 1635, in sage.matrix.matrix_gfpn_dense.mtx_unpickle
Failed example:
    N == mtx_unpickle(MS, 2, 5, t, True)           # optional: meataxe
Exception raised:
    Traceback (most recent call last):
      File "/home/jdemeyer/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 509, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/jdemeyer/sage-git/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 872, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.matrix.matrix_gfpn_dense.mtx_unpickle[18]>", line 1, in <module>
        N == mtx_unpickle(MS, Integer(2), Integer(5), t, True)           # optional: meataxe
      File "sage/matrix/matrix_gfpn_dense.pyx", line 1712, in sage.matrix.matrix_gfpn_dense.mtx_unpickle (/home/jdemeyer/sage-git/src/build/cythonized/sage/matrix/matrix_gfpn_dense.c:14936)
        raise ValueError(f"Expected a pickle with {FfCurrentRowSizeIo}*{nr} bytes per row, got {lenData} instead")
    ValueError: Expected a pickle with 3*2 bytes per row, got 16 instead
**********************************************************************

comment:63 in reply to: ↑ 62 Changed 3 years ago by SimonKing

Replying to jdemeyer:

On a 32-bit system, this gives:

sage -t --long --warn-long 87.3 src/sage/matrix/matrix_gfpn_dense.pyx
...
    ValueError: Expected a pickle with 3*2 bytes per row, got 16 instead
**********************************************************************

Thank you for testing on different architectures!

So, what shall we do with that test?

  • On the one hand, that test is about a pickling format that is known to be machine dependent and is therefore deprecated. That's an argument for removing it (which I would like to avoid).
  • On the other hand, I could try to do better and make unpickling work in that case as well.

I think I prefer the second way.

comment:64 Changed 3 years ago by git

  • Commit changed from 982755f103b4d1225051c060f93e9a4069deb87a to 78d7b21fe53577926bf0af4ab5dad5f426ca780a

Branch pushed to git repo; I updated commit sha1. New commits:

78d7b21MeatAxe: Enable unpickling of deprecated pickle created with different machine architecture

comment:65 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

Done! And I demonstrate that a deprecated pickle can be opened even if it was created on a silly machine with sizeof(long)==9.

comment:66 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Cool test! Now, we are still missing a doctest for the else case, when elif pickled_rowsize < FfCurrentRowSizeIo

comment:67 Changed 3 years ago by git

  • Commit changed from 78d7b21fe53577926bf0af4ab5dad5f426ca780a to ba72abac8a53b3d292df570d31709ab5c2f57157

Branch pushed to git repo; I updated commit sha1. New commits:

ba72abaTest another code path in unpickling meataxe matrices

comment:68 Changed 3 years ago by SimonKing

  • Status changed from needs_work to needs_review

Done!

There is now one test where the number of bytes is not divisible by the number of rows and one test where the number is divisible but too small to contain a matrix of the given dimensions. Together with the existing tests, I think it covers all code paths.


New commits:

ba72abaTest another code path in unpickling meataxe matrices

comment:69 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:70 Changed 3 years ago by vbraun

  • Branch changed from u/SimonKing/fix_pickling_of_matrix_gfpn_dense to ba72abac8a53b3d292df570d31709ab5c2f57157
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.