Skip to content
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

Intrument view cleanup proposal #1230

Closed
6 tasks done
nvaytet opened this issue Jun 30, 2020 · 5 comments
Closed
6 tasks done

Intrument view cleanup proposal #1230

nvaytet opened this issue Jun 30, 2020 · 5 comments

Comments

@nvaytet
Copy link
Member

nvaytet commented Jun 30, 2020

The instrument view code has become quite large, with many different functionalities, and is in dire need of a cleanup/refactor.

This outlines suggestions for the work to be done:

  • Split the code into multiple files, and put them into a sub-directory -> this has been done as part of Rewrite plotting #1324

  • Add unit testing for the different functions inside the instrument view -> this will be done as part of unit testing for all plotting code. See Refactor plotting code #1323 .

  • After internal discussions in the scipp team and also talking to LOKI scientists, we propose that the instrument view should be a purely 3D tool, leaving the 2D (spherical, cylindrical) projections to the classical 2D matplotlib plotting that is used elsewhere in scipp, via the use of groupby and realgned data. Using groupby is much more flexible than having a small set of predefined hard-coded projections, and removing the 2D projections from the instrument view code would simplify things quite a lot.

  • Axes labeling is currently not great, with the size of the tick labels depending on the pixel size, which does not work well in most cases:
    Screenshot_2020-06-30_11-37-35
    A maybe better solution would be to draw a box outline around the pixels, and add labels on the sides of the box. They would then be scaled according to the size of the box.

  • Camera initial lookAt and position could be based on the centre of the box outline, instead of being [0, 0, 0] by default, which should allieviate problems when an instrument has all its detectors far away from the origin (also the origin could be defined as the sample, or the source chopper, or anything, so always pointing at the origin makes little sense, we need to look at the detectors).

  • 2D projections in Mantid were used to unwrap detectors, making it possible in some cases to look at detectors that are buried inside a group of detectors (we are especially thiking at the side-by-side view which would ensure a 2D view with no overlapping pixels, as all other 2D views would still show overlap in the case of LOKI where straws are 'hiding' behind each other). An alternative to this could be to have some cut slices that can be moved through the instrument. A possible implementation for this would be to set the opacity of most pixels to something very low, and have a plane of high-opacity pixels, revealing the internals of deeply embedded detector arrangements. In addition, it is not clear how the Mantid side-by-side view would work for volumetric detectors. A quick mock up of this using pythreejs is shown below:
    Screenshot at 2020-06-30 12-15-42

@SimonHeybrock
Copy link
Member

  • The last item sounds like a very advanced feature and should probably moved to the bottom of the backlog.
  • What about the binning control? Is this the best solution, or is it another case of complicating things that could be solved by binning or slicing manually beforehand?
  • Can the binning slider be generalized (and at the same time simplified) by allowing sliders to attach to arbitrary dimensions, e.g., to allow for selections in combination with groupby or realign for selecting layers? I suppose the difference is that in one case we see different pixels, and in the other case just the intensities shown on the pixels.

@nvaytet
Copy link
Member Author

nvaytet commented Jul 1, 2020

The last item sounds like a very advanced feature and should probably moved to the bottom of the backlog.

Yes I know it sounds advanced. I actually got something basic, a proof of concept, working in the instrument view with just a few lines of code, so it's actually not so complex to achieve. It is however going against the current idea that we should be simplifying things, rather than adding more functionality.

It was just an idea I came up with when talking about how none of the different projections in Mantid work perfectly for Loki, and if we could come up with something new/different.

@nvaytet
Copy link
Member Author

nvaytet commented Jul 1, 2020

Can the binning slider be generalized (and at the same time simplified) by allowing sliders to attach to arbitrary dimensions, e.g., to allow for selections in combination with groupby or realign for selecting layers? I suppose the difference is that in one case we see different pixels, and in the other case just the intensities shown on the pixels.

One difficulty with this is that in pythreejs, when you have created a point cloud, you can't really change the number of points in the cloud without removing the first point cloud and then adding a new one to teh scene. And i suspect if you want to use this connected to a slider, the interaction will be super slow. I assume when you are selecting a new layer, the number of pixels will change.

Which is why I thought of changing the opacities via an RGBA color. You can even set the opacity to zero, which would look as if there are no other points there.

@SimonHeybrock
Copy link
Member

Which is why I thought of changing the opacities via an RGBA color. You can even set the opacity to zero, which would look as if there are no other points there.

This sounds like a good option then to combine with the groupby approach? If the instrument view would accepts arbitrary groupby-objects (i.e., what you get from groupby, before applying an operation on it), we could connect a slider to select the group.

@nvaytet
Copy link
Member Author

nvaytet commented Jul 1, 2020

The question on the binning is important I think.

On one hand, it would be really nice to have a range selection slider that you can also drag around, exactly like the slider in the Mantid instrument view (e.g. something that looks like the slider here: https://ghusse.github.io/jQRangeSlider/)
Annoying thing is, it does not yet exist in ipywidgets (jupyter-widgets/ipywidgets#1351).

On the other hand, we need to keep in mind the reproducibility aspect of things, and if we add too many widgets, the user can no longer know how he/she obtained the figure after playing with it for a while. Binning manually beforehand would help here.

Adaptive binning is something we talked about for realigned data also, so maybe doing so with such a slider would be nice, and could be introduced as a more generic feature, instead of purely for the instrument view.

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

No branches or pull requests

2 participants