Support downsampling of image data with NaNs by nup002 · Pull Request #3061 · pyqtgraph/pyqtgraph

nup002 · 2024-06-10T10:25:01Z

This PR adds tolerance for NaNs in ImageItem autodownsampling.

In the original code, NaNs in the image data would dominate after autodownsampling. With these changes, NaNs are ignored during downsampling.

pyqtgraph/functions.py

j9ac9k

The has_nans keyword argument is propagated into the recursive call on line 1776 in functions.py

pyqtgraph/functions.py

j9ac9k · 2024-06-11T02:56:11Z

Hi @nup002

I posted a couple of comments, the other thing I'm not sure about is if we want the keyword argument to be has_nans. It's a bit late now, but I'd be curious to know if there is anything similar in the library that we can have the keyword match. If nothing inside pyqtgraph, then is there anything in Qt that's equivalent and if not there, anything in numpy/scipy.

…call.

nup002 · 2024-06-11T07:35:04Z

Hi @nup002

I posted a couple of comments, the other thing I'm not sure about is if we want the keyword argument to be has_nans. It's a bit late now, but I'd be curious to know if there is anything similar in the library that we can have the keyword match. If nothing inside pyqtgraph, then is there anything in Qt that's equivalent and if not there, anything in numpy/scipy.

The only thing I can think of which is similar is Scipy's nan_policy: https://docs.scipy.org/doc/scipy/dev/api-dev/nan_policy.html
We can implement this instead of the simpler has_nans for finer control of the desired behaviour of downsample. But I am not sure I think this makes much sense since when downsampling the desired behaviour is very likely that NaNs must not propagate.

NilsNemitz · 2024-06-11T08:49:42Z

If there is a potential discussion on the desired behavior, then isn't the parameter really skip_nans instead of has_nans?

nup002 · 2024-06-11T11:17:35Z

Yes, but skip_nans implies a NaN check within downsample, and this is expensive to do every time it is called. Instead, the proposed implementation uses the self._imageHasNans attribute of ImageItem to avoid repeated NaN checks. I also do not feel that permitting NaN propagation in the downsampling makes much sense, except in certain edge cases.

NilsNemitz · 2024-06-11T16:58:59Z

Sorry, I misunderstood the discussion on the keyword naming, I thought this is exposed to the user. But the change is really that the existing 'has_nans' information is now passed on to the downsampling function which takes on a different behaviour then.

It seems to me that you are introducing a new downsampleMethod, like we already have in PlotDataItem:

pyqtgraph/pyqtgraph/graphicsItems/PlotDataItem.py

Line 406 in 8b60c0e

downsampleMethod ``str``, default ``'peak'``

Would it be reasonable to add a similar switch here, creating a user choice between downsampleMethod='mean' and downsampleMethod='nanmean'?

I tend to agree that the current behaviour is probably not the most helpful in most of the cases where NaN values might appear in the image, but it does seem at least mathematically valid and well defined.

nup002 · 2024-06-12T09:57:38Z

I propose that we expose an optional keyword argument to the user in ImageItem.__init__ named 'nan_policy' that defines how to handle NaNs during downsampling. The valid values would be 'propagate' which uses np.mean and 'ignore' which uses np.nanmean. We should at the same time add this functionality to PlotDataItem, which has the same limitation with regards to NaNs propagating into the downsampled data.

nup002 · 2024-06-15T19:48:40Z

I have implemented nanPolicy as an optional keyword argument for ImageItem which defaults to 'ignore', and which can be set with a the method ImageItem.setNanPolicy:

Control how NaN values are handled during downsampling for this ImageItem.

Parameters
----------
nanPolicy : str
    Must be one of ['ignore`, 'propagate']. If 'nanPolicy' is 'ignore', NaNs
    are automatically ignored during downsampling, at the expense of
    performance. If 'nanPolicy' is 'propagate', NaNs are kept during
    downsampling. ImageItem is instantiated with nanPolicy set to 'ignore'.

If this feature request is approved, I will work on adding matching functionality to PlotDataItem so that it also can ignore NaNs during downsampling.

NilsNemitz · 2024-07-02T11:26:25Z

pyqtgraph/functions.py



-def downsample(data, n, axis=0, xvals='subsample'):
+def downsample(data, n, axis=0, xvals='subsample', *, nanPolicy='ignore'):


renaming 'nan_policy' to 'nanPolicy' seems like a good idea for matching our Qt-inspired case style.
Shouldn't 'ignore' be 'omit' if we are aiming to resemble SciPy conventions?

Do we have any additional opinions for what should be the default policy?
Although I agree that 'omit' is probably the most commonly useful policy, maybe we should stick with 'propagate' as the default to better match existing behavior.

pyqtgraph/functions.py

NilsNemitz

Dear @nup002 , sorry for the delay.

I like the approach of matching the SciPy solution, and I think that's a fantastic upgrade for ImageItem.

I am not usually doing code review, but I've added a few comments.

I think the main items are that 'ignore' is 'omit' in the SciPy convention, and that it is probably best to leave the default behavior on 'propagate' for backwards compatibility and possibly speed.

The documentation comments are not too serious, ImageItem will need a documentation pass in the future. If you want someplace to look for reference, the most up-to-date piece of documentation style we have is probably PlotDataItem after PRs #3039 and #3057 .

NilsNemitz · 2024-07-02T11:40:34Z

pyqtgraph/functions.py

+    elif nanPolicy == 'propagate':
+        d2 = d1.mean(axis+1)
+    else:
+        raise ValueError(f"Keyword argument {nanPolicy=} must be one of ['propagate', 'ignore'].")


@j9ac9k has been looking into documentation styles recently and is clearer on the conventional markups, but I think this might be "Keyword argument 'nanPolicy' must be one of 'propagate' or 'ignore'."
And this might not need an f string?

If a keyword only argument can be one of several things (in this case string) the syntax is:

{ 'option_1', 'option_2' }

You can see an example of it here (but you have to scroll down a bit) https://numpydoc.readthedocs.io/en/latest/format.html#parameters

NilsNemitz · 2024-07-02T11:41:01Z