Gui improvements #148

jokasimr · 2025-04-15T12:40:44Z

Corrected logic in display function
Added detector view tab
Nicer formatting of table columns
Added default y-scale limit in reflectivity figure

…s with rounding

Copilot

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

src/ess/reflectometry/gui.py

nvaytet · 2025-04-23T08:51:24Z

src/ess/reflectometry/gui.py

+        self.working_label = widgets.Label(
+            "...working", layout=widgets.Layout(display='none')
+        )
+        self.widget = widgets.HBox(


For this and the other classes you have that have a self.widget = HBox(...), you might want to look at how we make custom widgets in Plopp, so you can directly embed them into other widgets, without having to use the .widget attribute: https://github.com/scipp/plopp/blob/main/src/plopp/widgets/checkboxes.py

nvaytet · 2025-04-23T08:53:41Z

src/ess/reflectometry/gui.py

+        workflow[ZIndexLimits] = (0, 16 * 32)
+        workflow[WavelengthBins] = sc.geomspace(
+            'wavelength',
+            2,
+            13.5,
+            2001,
+            unit='angstrom',
+        )


I guess these are not taking into account what is set in the tab where you have Z and L parameters in a table.
Should it be in sync?

Also, 2000 wavelength bins is quite a lot, we could make the figures lighter by reducing that?

I guess these are not taking into account what is set in the tab where you have Z and L parameters in a table.
Should it be in sync?

Yes that's right. No I don't have the impression they should be in sync. This view is independent from the reduction settings.

Also, 2000 wavelength bins is quite a lot, we could make the figures lighter by reducing that?

I think 2000 is a good setting for now. In the future this might be a user defined setting. For now I think we shouldn't tweak this before getting some feedback from the ids.

nvaytet · 2025-04-23T08:54:42Z

src/ess/reflectometry/gui.py

+        da.bins.data[...] = sc.scalar(1.0, variance=1.0, unit=da.bins.unit)
+        da.bins.unit = 'counts'


I am not sure why we need to do this here? Can we not histogram the data in ReducibleData ?

Not sure what you mean here. If you're asking why we're setting the weights to 1, it's because we don't want any weights applied, we just want the detector counts.

nvaytet · 2025-04-23T08:55:09Z

src/ess/reflectometry/gui.py

+        da.bins.data[...] = sc.scalar(1.0, variance=1.0, unit=da.bins.unit)
+        da.bins.unit = 'counts'
+        da.masks.clear()
+        da.bins.masks.clear()


Why do we need to remove masks? Is the workflow setting masks somewhere?
I guess we have no interface to add masks in the GUI right now.
If there are masks on the data, they should be visible on the figure, no?

Yes the workflow is adding masks to the data.

In this case we don't want to view the masks, we only want the raw count data.
That's also the reason whey the counts are set to 1.0, we don't want any corrections applied.

If you just want raw counts, it sounds like we should compute another result from the workflow than the ReducibleData[SampleRun] (e.g. RawDetectorData)?

Computing something and then settings values to 1 seems strange to me. I still don't understand why you want to do that above...
Is it because you need the coordinates to compute the wavelengths to make the figure that you are using ReducibleData?
Maybe it's a sign that there should be an additional step in the workflow that splits this up, so you can compute a raw data with coordinates before masks and corrections are added?
Not sure if this would work with the current order in which operations are applied.

If you just want raw counts, it sounds like we should compute another result from the workflow than the

Problem is that RawDetectorData doesn't have wavelengths.

Is it because you need the coordinates to compute the wavelengths to make the figure that you are using ReducibleData?

Yes

Maybe it's a sign that there should be an additional step in the workflow that splits this up, so you can compute a raw data with coordinates before masks and corrections are added?

We could do that. But then you could say, what if I just want those coordinates and not those, what if I want those corrections but not those, what if I want those masks but not those, etc, and we would need to define a whole range of "domain types" for each of the combinations. That said, maybe this particular case is common enough that we do want to introduce a separate type that can be used here.

Not sure if this would work with the current order in which operations are applied.

It would work 👍

I agree it's a bit odd to set the weights to 1 and clear all masks. But on the other hand, concretely, what is the drawback? (besides performance, which is probably not that relevant)

The drawback is that the GUI is starting to feel (at least to me) like it contains quite a few hacks.
However, because I don't want to get too deeply involved in the GUI side of things, I'll let you decide what you want to do.

The drawback is that the GUI is starting to feel (at least to me) like it contains quite a few hacks.

That's a concern we should take seriously and try to adress.

If the issues you're referring to are not directly related to the changes in this PR I think we should adress them in separate PRs. If you have concrete problems or changes in mind feel free to open issues to fix those. If your concerns are too vauge to be directly converted to issues, let's sit down have a chat about them and see if we can figure something out.

Like I said above, I'm not against adding a DetectorDataWithCoords domain type. I just don't see the advantage, and I see that adding more domain types complicates the package and makes it more confusing for users, so if possible I'd prefer to avoid it.

nvaytet · 2025-04-23T08:57:41Z

src/ess/reflectometry/gui.py

+        self.tabs.set_title(len(self.tabs.children) - 2, "Nexus Explorer")
+        self.tabs.set_title(len(self.tabs.children) - 1, "Detector View")


use self.tabs.titles = [ SOME LIST OF NAMES ] ?

nvaytet · 2025-04-23T11:38:26Z

src/ess/reflectometry/gui.py

+        def on_tab_change(change):
+            old = self.tabs.children[change['old']]
+            new = self.tabs.children[change['new']]
+            if hasattr(old, 'is_active_tab'):


I guess you could even leave the hasattr check out, just setting the attribute on the old object, even if it doesn't exist to begin with, should be allowed?

What's the benefit?

Simpler code?

If the simplification is removing the hasattr checks, at the cost of adding attributes to random objects, that's not worth it in my opinion.

I wouldn't call them completely 'random', but it was just a suggestion. You can take it or leave it.

Sorry that came of as dismissive, I didn't mean it like that. I appreciate your suggestions and the effort you put into the review

nvaytet · 2025-04-23T11:39:21Z

src/ess/reflectometry/gui.py

        self.tabs.set_title(1, "Settings")
        self.tabs.set_title(2, "Log")


Also use .titles = [...]?

jokasimr added 3 commits April 15, 2025 11:52

fix: take reference settings into consideration

40063a8

fix: take reference settings into consideration + add column renderer…

03b742a

…s with rounding

feat: add detector view

f93c632

jokasimr added this to Development Board Apr 15, 2025

github-project-automation bot moved this to In progress in Development Board Apr 15, 2025

jokasimr moved this from In progress to Selected in Development Board Apr 15, 2025

feat: group by angle in reference table

0ea4e3f

MridulS requested a review from Copilot April 16, 2025 08:39

Copilot AI reviewed Apr 16, 2025

View reviewed changes

src/ess/reflectometry/gui.py Outdated Show resolved Hide resolved

src/ess/reflectometry/gui.py Outdated Show resolved Hide resolved

fix: safer plot retry + bug in 'in progress' message

63ab0a1

nvaytet reviewed Apr 23, 2025

View reviewed changes

feat: only rerun detector view when the detector view is in tab

eb1406f

jokasimr requested a review from nvaytet April 23, 2025 10:50

nvaytet reviewed Apr 23, 2025

View reviewed changes

fix: set titles

c923cb2

jokasimr mentioned this pull request Apr 23, 2025

Fix selection bug in case of sorting / filtering #149

Merged

nvaytet approved these changes Apr 24, 2025

View reviewed changes

Merge branch 'main' into select-reference

69a9eef

jokasimr enabled auto-merge April 24, 2025 11:16

jokasimr merged commit 7d130c8 into main Apr 24, 2025
4 checks passed

jokasimr deleted the select-reference branch April 24, 2025 11:23

github-project-automation bot moved this from Selected to Done in Development Board Apr 24, 2025

		da.bins.data[...] = sc.scalar(1.0, variance=1.0, unit=da.bins.unit)
		da.bins.unit = 'counts'

		self.tabs.set_title(len(self.tabs.children) - 2, "Nexus Explorer")
		self.tabs.set_title(len(self.tabs.children) - 1, "Detector View")

		self.tabs.set_title(1, "Settings")
		self.tabs.set_title(2, "Log")

Gui improvements #148

Gui improvements #148

Uh oh!

Conversation

jokasimr commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokasimr Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jokasimr Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jokasimr commented Apr 15, 2025 •

edited

Loading

jokasimr Apr 23, 2025 •

edited

Loading

jokasimr Apr 23, 2025 •

edited

Loading