search

pyplati / platipy

Processing Library and Analysis Toolkit for Medical Imaging in Python
https://pyplati.github.io/platipy/
Apache License 2.0
103 stars 24 forks source link

Replace np.product with np.prod pending deprecation #221

Closed StellarStorm closed 11 months ago

StellarStorm commented 11 months ago

First off, thanks for a very useful tool!

This PR should help future-proof for the release of Numpy 2.0 while not changing any actual behavior of platipy. Currently, while using the latest Numpy (1.25), I get deprecation messages like this:

C:\Users\<redacted>\AppData\Local\Temp\ipykernel_26204\2109769358.py:19: DeprecationWarning: `product` is deprecated as of NumPy 1.25.0, and will be removed in NumPy 2.0. Please use `prod` instead.
  leaky_dvh = calculate_dvh_for_labels(leaky_dose, dvh_structures)

Here is the relevant deprecation notice for Numpy 1.25: https://numpy.org/devdocs/release/1.25.0-notes.html#deprecations

This change should not break compatibility with old versions, as np.prod has been a method since at least v1.10.1 (probably earlier), while platipy currently requires v1.10.2 as the minimum numpy version https://docs.scipy.org/doc/numpy-1.10.1/reference/generated/numpy.prod.html#numpy.prod

Also this change should be purely cosmetic. In the numpy pull request to deprecate np.product, it is replaced seamlessly by np.prod https://github.com/numpy/numpy/pull/23314/files

pchlap commented 11 months ago

Thanks a lot for this contribution @StellarStorm! Very much appreciated. This is all looking really good, however the automated tests seem to be failing due to some unrelated reason. I am traveling at the moment so I haven't got time to resolve that issue just now, but I will take a look in the next few days and then merge this through once we can confirm that all tests are passing. Thanks!!

StellarStorm commented 11 months ago

Sounds great! I took a quick peek at the test logs and it looks like the issue may have something to do with sklearn trying to be installed. sklearn is a dummy package that points to scikit-learn and says to install it instead . sklearn was recently deprecated and installation is on a brownout schedule where installing raises an exception every so often, so that could be killing the tests.

EDIT: I'm not positive of your environment, but these might have something to do with it:

https://github.com/pyplati/platipy/blob/f1f6b100e41cf508aff8dc02dfb5b44db1251a68/poetry.lock#L2432

https://github.com/pyplati/platipy/blob/f1f6b100e41cf508aff8dc02dfb5b44db1251a68/poetry.lock#L3820

pchlap commented 11 months ago

Hi @StellarStorm, yes I think you're spot on about the sklearn library name being deprecated which caused these tests to fail. However on rerun they are passing for now. So I am happy to merge this and I will address updating the sklearn package name in a future pull request.

Thanks again for your contribution! Really appreciate it :)