-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Display QPI image data #146
base: master
Are you sure you want to change the base?
Display QPI image data #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulmueller when you get a chance, pleae have a look at the changes :)
I tested that it works on normal rtdc files, and on rtdc files with qpi data.
Perhaps I need to add some tests?
else: | ||
raise ValueError(f"Options for `feat` are 'image', " | ||
f"'qpi_pha' and 'qpi_amp', got {feat}") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generalised this a bit. Check if there is anything more to do :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not convert the float32 images to uint8. This could make things look really ugly (the kind of artifacts you see in .gif images) and pyqtgraph should support showing float32 images. Also, for the phase images you would like to use the diverging colormap coolwarm (needs to be ported to ShapeOut without adding the matplotlib dependency). So ideally, you have a background phase of 0 and then a symmetric colormap with positive (red) and negative (blue) phase delay.
# the plot got updated, and we still have the old data | ||
cellimg, imkw = self.get_event_image(self.rtdc_ds, 0) | ||
|
||
self.imageView_image_poly.hide() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can hide first and have that as default, and then call .show()
if the image is available (see below)
shapeout2/gui/quick_view/qv_main.py
Outdated
if feat == "image": | ||
self.imageView_image_poly.setImage( | ||
cellimg, **imkw) | ||
self.imageView_image_poly.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that this is sliiiightly different than show_event
. I guess I could add a groupBox for the poly images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there should be a separate class entirely that handles image plotting? Generalization is not easy due to the different data types and requirements for visualization.
shapeout2/gui/quick_view/qv_main.py
Outdated
if feat in ds: | ||
cellimg, imkw = self.get_event_image(ds, event, feat) | ||
if feat == "image": | ||
self.imageView_image.setImage(cellimg, **imkw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These if statements also annoy me a bit. Is there a way to access the QT attributes via string definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could generalize it with dictionaries defined in __init__
:
self.img_info = {
"image": [self.imageView_image, "gray"],
"qpi_pha": [self.imageView_image_pha. "coolwarm"],
etc...
}
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #146 +/- ##
==========================================
- Coverage 78.93% 78.71% -0.22%
==========================================
Files 63 63
Lines 6768 6807 +39
==========================================
+ Hits 5342 5358 +16
- Misses 1426 1449 +23 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main issue that I see is that the current way contours are plotted is outdated and also does not work with phase data (changing pixels in an RGB image). So as far as I can see, there is no way around rewriting displaying the contour for phase and amplitude data.
Also, I believe there should be some way of setting the range for (at least) phase values. And phase values should always be centered at zero with the corresponding colormap showing white or no color. "coolwarm" from matplotlib is a good way of displaying phase, but you would have to copy-paste it to work with pyqtgraph, because I think it is not (yet) in pyqtgraph. "viridis" got ported to pyqtgraph, so it should be straight-forward.
else: | ||
raise ValueError(f"Options for `feat` are 'image', " | ||
f"'qpi_pha' and 'qpi_amp', got {feat}") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not convert the float32 images to uint8. This could make things look really ugly (the kind of artifacts you see in .gif images) and pyqtgraph should support showing float32 images. Also, for the phase images you would like to use the diverging colormap coolwarm (needs to be ported to ShapeOut without adding the matplotlib dependency). So ideally, you have a background phase of 0 and then a symmetric colormap with positive (red) and negative (blue) phase delay.
shapeout2/gui/quick_view/qv_main.py
Outdated
return cellimg, imkw | ||
|
||
@staticmethod | ||
def display_contour(ds, event, state, cellimg): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, we might want to change how we compute/display the contour, i.e. we are planning to use non-discrete contours (@SaraKaliman). So it might make sense to implement two functions add_contour_to_image
(for "image" data) and draw_contour_on_plot
(for float32 qpi data) which could then also be used later for non-discrete contours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Practically I want to use shapeout to visualise the image data so Kyoo can look at the data in one place. For that, what I have built right now is enough. The colormaps and contour issues will just hold that up. Could we put all that in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is just for Kyoo, then you could just create a custom executable for him:
cd build_recipes
pip install -r win_build_requirements.txt
pyinstaller -y --log-level=WARN win_ShapeOut2.spec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not like to merge an incomplete feature. Moreover, the grayscale colormap leads to ambiguities. This should not be used by the general crowd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it isn't a complete feature then I understand. I'll try to implement the colorbar etc soon.
Thanks for the custom executable, much better... I was just gonna do the dev version. :)
shapeout2/gui/quick_view/qv_main.py
Outdated
if feat == "image": | ||
self.imageView_image_poly.setImage( | ||
cellimg, **imkw) | ||
self.imageView_image_poly.show() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe there should be a separate class entirely that handles image plotting? Generalization is not easy due to the different data types and requirements for visualization.
shapeout2/gui/quick_view/qv_main.py
Outdated
if feat in ds: | ||
cellimg, imkw = self.get_event_image(ds, event, feat) | ||
if feat == "image": | ||
self.imageView_image.setImage(cellimg, **imkw) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could generalize it with dictionaries defined in __init__
:
self.img_info = {
"image": [self.imageView_image, "gray"],
"qpi_pha": [self.imageView_image_pha. "coolwarm"],
etc...
}
@@ -203,7 +203,7 @@ | |||
</size> | |||
</property> | |||
<property name="currentIndex"> | |||
<number>0</number> | |||
<number>2</number> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep 0 the default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea how that changed. QT seems to change random stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you click on a tab in QtDesigner, it sets this tab as default.
As described in (create issue) I want to see qpi data non-scalar features in shapeout2.