Skip to content

Conversation

@SwayamInSync
Copy link
Member

This PR adds the pickle support as per #231
Usage:

original = np.array([1.5, 2.5, 3.5, 4.5], dtype=QuadPrecDType(backend=backend))
# Pickle and unpickle
pickled = pickle.dumps(original)
unpickled = pickle.loads(pickled)

Using np.savez

arr1 = np.array([1.5, 2.5, 3.5], dtype=QuadPrecDType(backend=backend))
arr2 = np.array([[1.0, 2.0], [3.0, 4.0]], dtype=QuadPrecDType(backend=backend))

np.savez(fname, array1=arr1, array2=arr2)
loaded = np.load(fname, allow_pickle=True)	# allow_pickle must be True during loading
loaded_arr1 = loaded['array1']
loaded_arr2 = loaded['array2']

@SwayamInSync
Copy link
Member Author

We might not need to use NPY_LIST_PICKLE as when NumPy pickles a QuadPrecDType array:

  • It saves the dtype (using our reduce) (QuadPrecDType, ('sleef',))
  • It saves the shape
  • It saves the data, PyArray_ToString returns raw 128-bit bytes

StringDType needs NPY_LIST_PICKLE because:

  • Its memory contains pointers to Python strings
  • PyArray_ToString would just copy worthless pointer values
  • So it uses _getlist_pkl to extract actual string objects

Our QuadPrecDType stores actual numeric values in memory, so PyArray_ToString works perfectly

@SwayamInSync
Copy link
Member Author

hash implementation might require some careful handling https://github.com/python/cpython/blob/20b69aac0d19a5e5358362410d9710887762f0e7/Python/pyhash.c#L87

I'll do this in a different PR and then we can close the #231

Copy link
Member

@jorenham jorenham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stub changes looks good to me

@juntyr
Copy link
Contributor

juntyr commented Nov 22, 2025

Why is the backend provided as an integer? A string literal might be a lot clearer, or if it has to be an int maybe we could add an int enum?

@SwayamInSync
Copy link
Member Author

Why is the backend provided as an integer? A string literal might be a lot clearer, or if it has to be an int maybe we could add an int enum?

Internally backend is defined as enum, we provide the friendly API for the user to add input as string. For the sake of static tools, IDE suggestions I think defining as enum might make sense maybe @jorenham can give some inputs here

@jorenham
Copy link
Member

I suppose you could do Backend: typing.TypeAlias = typing.Literal[1, 2], or maybe even an enum.IntEnum

Co-authored-by: Joren Hammudoglu <jhammudoglu@gmail.com>
@SwayamInSync
Copy link
Member Author

@ngoldbaum a gentle reminder here for review

@ngoldbaum
Copy link
Member

@SwayamInSync I'd appreciate it if you could give me more than a couple of days - especially over a weekend - before pinging me on PR reviews. I had your PRs on my to-do list.

Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C code and tests overall look good. I didn't review the tests in detail but I'm glad to see such extensive coverage of usages.

assert unpickled.flags.f_contiguous == original.flags.f_contiguous

@pytest.mark.parametrize("backend", ["sleef", "longdouble"])
def test_pickle_reduce_method(self, backend):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment is that this test is a little too tightly coupled to the implementation to be useful.

You could, for example, decide to use __getnewargs__ instead of __reduce__ but then you'd have to update this test too. https://docs.python.org/3/library/pickle.html#object.__getnewargs__.

Copy link
Member Author

@SwayamInSync SwayamInSync Nov 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think I should just remove this test? as we already have tests which are just focussed on working of the pickle rather than implementation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm suggesting to delete the test.

@SwayamInSync
Copy link
Member Author

@SwayamInSync I'd appreciate it if you could give me more than a couple of days - especially over a weekend - before pinging me on PR reviews. I had your PRs on my to-do list.

So sorry, I was working this weekend as well so slightly forgot the track of days :)

@SwayamInSync
Copy link
Member Author

Merging this in

@SwayamInSync SwayamInSync merged commit f2a4bec into numpy:main Nov 26, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants