vivarium-collective / vivarium-core

Core Interface and Engine for Vivarium
https://vivarium-core.readthedocs.io/
Apache License 2.0
26 stars 3 forks source link

serialize.py: np.float128 and np.complex256 are not implemented on some platforms #196

Closed cyrus-bio closed 2 years ago

cyrus-bio commented 2 years ago

np.float128 and np.complex256 do not exist for numpy on some platforms (including Windows, ARM-based MacOS, which breaks scripts when trying to import them.

Deleting np.float128 and np.complex256 on lines 191, 192 in serialize.py fixed the issue for me. https://github.com/vivarium-collective/vivarium-core/blob/aae63ff9dc3d26a2fea8fb8d74ccf58bce24fe67/vivarium/core/serialize.py#L191

A couple of other examples: https://github.com/mpi4jax/mpi4jax/issues/112 https://github.com/winpython/winpython/issues/613

U8NWXD commented 2 years ago

It looks like float64 is the default for numpy, so we'd only get np.float128 if someone specified that explicitly, which seems unlikely. We could remove these and note in the docs that we don't support those data types?

eagmon commented 2 years ago

who put those in and why? Can you check the git blame @cyrus-bio?

Maybe we can check if the type exists, and only use it if it does?

cyrus-bio commented 2 years ago

From a cursory search it seems that most people just use np.float64 at most, or do something along the lines of: if hasattr(np, 'float128'): do A else: do B

U8NWXD commented 2 years ago

I think I put those in. They just tell Vivarium that the numpy serializer should be used for objects of type np.float128. If someone does provide a np.float128 object after we remove the np.float128 reference from this file, nothing will break. Vivarium will just take slightly longer to find the right serializer (it will loop through the serializers instead of looking the right serializer up in a dictionary)

eagmon commented 2 years ago

Sounds like we can safely remove it then. @cyrus-bio -- want to open a pull request in vivarium-core?