search

BUNPC / pysnirf2

Python package for reading, writing and validating Shared Near Infrared Spectroscopy Format (SNIRF) files
GNU General Public License v3.0
15 stars 11 forks source link

BUG: Fix for deprecated numpy type #21

Closed larsoner closed 2 years ago

larsoner commented 2 years ago

np.float is a deprecated alias for float:

DeprecationWarning: `np.float` is a deprecated alias for the builtin `float`. To silence this warning, use `float` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.float64` here.

I also think this was probably meant to be np.float32 anyway, not np.float (which corresponds to a 64-bit float on Python, at least on macOS/Linux/Windows). This puts np.float32 in explicitly. It also removes a bunch of trailing whitespace since this seems to be the norm in scientific Python computing, but I can roll back that change if desired!

sstucker commented 2 years ago

Good catch: This should be np.float64, though, per the SNIRF spec, which wants a double for all floating point scalars.

https://github.com/fNIRS/snirf/blob/master/snirf_specification.md#introduction

To actually make the changes in the library, run gen.py from the project directory... this generates the new library pysnirf2.py using the templates and boilerplate, including the header you changed here.

I could add an action to automatically do this I suppose

larsoner commented 2 years ago

Okay just removed np.float32 so the list is now [float, np.float64], then ran python gen/gen.py

sstucker commented 2 years ago

In the case of long ints, I show a warning: https://github.com/BUNPC/pysnirf2/blob/00e1ed588fc696bfc54f9b163feab7ed244a06fb/gen/header.py#L653

As the spec document specifies 32 bit ints and 64 bit floats.

We could add similar logic and a warning here.

sstucker commented 2 years ago

Oh, it appears I'm mistaken-- the spec doesn't care about 32 vs 64 bit floats.

Maybe I invent that policy in my PR here: https://github.com/fNIRS/snirf/pull/99

I will consider removing the requirement for 64 bit floats