Skip to content

ImageView: make .nframes() to use .axis['t'] instead of .shape[0]#2623

Merged
j9ac9k merged 1 commit intopyqtgraph:masterfrom
sem-geologist:patch-3
Mar 3, 2023
Merged

ImageView: make .nframes() to use .axis['t'] instead of .shape[0]#2623
j9ac9k merged 1 commit intopyqtgraph:masterfrom
sem-geologist:patch-3

Conversation

@sem-geologist
Copy link
Copy Markdown
Contributor

@sem-geologist sem-geologist commented Feb 23, 2023

BUG-FIX:
This is fixing the behavior of ImageView for 3 dimensional images, where stacking of images or time frames are not at commonly expected 0'th index of arrays shape. Using hard-coded image.shape[0] causes the progress of play stop at such index if image.shape[0] is smaller than the shape at index set for axis t.

Alternatively, would not it be better to remove this method and use where needed self.tVals.shape[0] ? From performance point of view that would be probably better, but seeing the checking if self.image is None: probably there is an external reason for cases where would be 0 frames (in the end this method is public and could be called externally). Thus I had added at the end the additional case returning 1 when there is single frame and so nframes() should cover all possible cases also then called externally.

Minimal Code to reproduce the BUG:

import numpy as np
import pyqtgraph as pg

app = pg.Qt.App([])

imv = pg.ImageView()
data = (np.random.random((16,16,32)) * 254).astype(np.uint8) # x,y,z

imv.setImage(data, axes={"t": 2, "x": 0, "y": 1})
imv.show()
print(imv.nframes())
imv.play(4)

.play() or keyboard keys will go as far as 16 slice, because nframes() returns image.shape[0] which is 16.

also

import numpy as np
import pyqtgraph as pg

app = pg.Qt.App([])

imv = pg.ImageView()
data = (np.random.random((16,16)) * 254).astype(np.uint8) # x,y

imv.setImage(data, axes={"x": 0, "y": 1})
imv.show()
print(imv.nframes())

will print 16, and there is only 1 frame.

This bug-fix makes the first example to play to the end, and second example to correctly return 1 as number of frames

This is fixing the behviour of ImageView for 3 dimentional images, where stacking or time is not in 0 index of array shape.
as nframes is being called by other methods which are enbled only if axes t is recognised, there is no need to check for if axes['t] is None (then frame should be 1);

alternatively, would not it be better to remove this method and use where needed `self.tVals.shape[0]` ? From performance point of view that would be better, but seeing the checking `if self.image is None:` probably there is the reason for cases where would be 0 frames. Thus I had added at the end `return 1` and so `nframes()` should cover all possible cases also then called externally.
@sem-geologist sem-geologist changed the title FIX: ImageView nframes method is too hardcoded ImageView: modify nframes() to use axis['t'] index instead of 0 Feb 23, 2023
@sem-geologist sem-geologist changed the title ImageView: modify nframes() to use axis['t'] index instead of 0 ImageView: modify .nframes() to use .axis['t'] instead of 0 Feb 23, 2023
@sem-geologist sem-geologist changed the title ImageView: modify .nframes() to use .axis['t'] instead of 0 ImageView: make .nframes() to use .axis['t'] instead of 0 Feb 23, 2023
@sem-geologist sem-geologist changed the title ImageView: make .nframes() to use .axis['t'] instead of 0 ImageView: make .nframes() to use .axis['t'] instead of .shape[0] Feb 24, 2023
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Feb 26, 2023

@outofculture @campagnola you two work with these more than I do, comments?

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Feb 26, 2023

thanks for the PR @sem-geologist the diff LGTM, but this is a part of the library I don't work with much myself so hopefully one of the other maintainers will chime in here.

@sem-geologist
Copy link
Copy Markdown
Contributor Author

I mentioned initially the performance concern; actually I see no noticeable performance hit (internally .nframes method is used only when using built-in play through frames by triggering/keeping pressed left, right, up, down, PgUp, PgDown and SpaceBar keyboard keys;). It extends the function only by additional two dict look-ups (.axes and ["t"]). However, it makes .nframes to behave as intended instead erroneous in cases then 't' axis does not correspond to .image.shape[0]. I updated the initial comment above with small working examples to demonstrate where does the pyqtgraph errors currently, and what this bug-fix addresses.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Mar 3, 2023

I think I've given folks ample opportunity, thanks for the explanation @sem-geologist this LGTM.

@j9ac9k j9ac9k merged commit 2ea466b into pyqtgraph:master Mar 3, 2023
@sem-geologist
Copy link
Copy Markdown
Contributor Author

cool, thanks. I saw it made into newest version and is ready in 0.13.2. I can now get rid of subclass in my new project which uses pyqtgraph: https://github.com/sem-geologist/FIB-data-cleaner . (That is still kind of WIP)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants