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

Expand all for Screens #39

Open
wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Conversation

chris-allan
Copy link
Member

Loads the metadata for all relevant metadata when an SPW container
is selected.

All of the AJAX calls have been converted to Axios, follow an
asynchronous paradigm, are cancellable, and where appropriate now
have loading spinners.

/cc @emilroz

@chris-allan
Copy link
Member Author

First pass.

New Axios data loading infrastructure now applies to all data types. You should definitely have a nicer, cleaner and more visible status when working with PDI. I'll be working on adjusting the filter and data loading stack to support Screens now.

/cc @emilroz

@will-moore
Copy link
Member

In Layout.js, this.props.fieldId is now undefined, so loading right panel on Well click fails...

    setSelectedWells(wellIds) {
        this.setState({selectedWellIds: wellIds});
        // Trigger loading Wells in right panel...
        var well_index = this.props.fieldId;
        var selected_objs = wellIds.map(wId => ({id: 'well-' + wId, index: this.props.fieldId}))
...

since we get url like /webclient/metadata_details/well/178/ without ?index=0 which gives an error. Clearly, webclient should handle a missing index a bit better, but in the meantime we need an index.
Also the line var well_index = this.props.fieldId; is redundant (probably was before too).

if (selectedNode.type === "acquisition") {
const plateNode = this.props.jstree.get_node(selectedNode.parent);
nodes = [plateNode];
fieldId = Math.min(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Math.min(array) returns NaN annoyingly!
Need something like .reduce((p, i) => Math.min(p, i))

@will-moore
Copy link
Member

Testing locally with development server, I see some slightly strange sequence when clicking on a Screen (with 4 Plates) which already viewing a Screen (with 2 Plates, displayed in centre):

Basically, the selection change event in the tree triggers re-loading of thumbnails for the 2 Plates already displayed (even though a different Screen has just been selected) so I see thumbnail spinners and thumbnails loading right before the old 2 Plates are replaced by the 4 new Plates (when those AJAX calls return)

screen shot 2018-05-15 at 11 08 05

Sequence is hard to read as URLs are not shown, but after /dataproviders/ url are 4 calls for fields for new Plates, then the 2 thumbnail requests for the old Plates, then the 4 webgateway/plate/id/0/ calls for the new Plates and finally the 4 thumbnail calls for those Plates.

I'll maybe hold off more testing for now until deployed on web-dev-merge.

@chris-allan
Copy link
Member Author

There is definitely some weird stuff happening when you switch back and forth. Especially when running on a server where there is a single thread, Chrome is making 6 requests in parallel, and a lot of cancellation needs to happen.

Let me finish up making the filtering and data loading Screen aware and I'll switch back to looking at that.

@chris-allan
Copy link
Member Author

Should now be much more solid and ready for tomorrow's testing on web-dev-merge.

@will-moore
Copy link
Member

Testing the heatmaps on Dataset, it took me a while to see the SETTINGS control at the bottom. I only found it on the UI after first reading the code to see that it's in the footer. I think it needs to be at the top somewhere otherwise it will be missed. Possibly at the right of the "Add Table Data" row?

I'm not sure what the "Normal" option is for? Not loading thumbnails or heatmap. Is this to turn thumbnails off so we don't load too many for big plates?
Maybe the thumbnails placeholders should be black (or darker?). It somehow seems wrong with grey, like we're waiting for them to load?

screen shot 2018-05-16 at 22 56 20

It sounds like "Normal" should be the default setting. But actually "Thumbnails" is the default, right?
Maybe you don't need a "Normal" option, you can just toggle off the "Thumbnails" option? (and replace Thumbnails with Heatmap if that is chosen)?

On the List view, the SETTINGS menu doesn't work, but I'm still not seeing thumbnails (no way to turn them back on in this view)

It feels like the HeatMap options should allow me to choose from the same list as "Add Table data..." dropdown (to choose from options that I haven't yet loaded, but would be loaded when I request them).

I'm trying to think of how to have heat-maps not behave so differently in List view vv Thumbnails view. It seems wrong that I can choose a column in the List view and check the checkbox to show Heatmap, but when I go to Thumbnails view I don't see that heatmap - then I go back to List view and the Heatmap has gone.
The key problem is that List view allows multiple Heatmaps but Thumbnails view only ONE. We could limit the List view to only allow a single Heatmap, but I think it is useful to be able to compare heatmaps from multiple columns.
Needs some more thought...

I like the numbers in the thumbnails for heatmap, although this also needs formatting to 2 decimal places for floats (not necessarily in this PR).

@will-moore
Copy link
Member

Minor point: The spinners while loading Filters or Table data are shown to the left of the Filter label and "Add Table data..." chooser, so when the spinner is shown/hidden, these elements jump to the side.
It's more noticeable for the "Add Table data..." chooser than for the filters. Showing the spinner to the right of this would make it a little smoother.

@will-moore
Copy link
Member

Not related to this PR (except wrt trying to improve UI clarity):
The "Add table data..." label and the "Table_* " options within it both use the term "Table" to refer to different things (parade table/list view vv OMERO.Tables), which is confusing. Also, you now have to use "Add table data..." for heatmap, even if you never display this data in a table. So maybe this should just be "Load data..." ?

@chris-allan
Copy link
Member Author

Based on the feedback above:

  • The "Settings" menu is now part of the Layout component and is visually located at the top of the plugin next to the layout selector
  • "Normal" and "Heatmap" modes are now below a top level "Mode" menu item; "Normal" is effectively there to allow for the selection of no heatmap
  • Thumbnail placeholders for Plate data are now very obvious to distinguish the various states
  • Toggling on and off Dataset grouping and thumbnail display are now below a top level "View" menu item
  • The "Settings" menu should now work across all layouts although it is still a bit weird on the "List" and "Plot" layouts; this will require some more design thought
  • Loading spinners are now to the right of their associated controls
  • "Add table data..." is now "Add data..."
  • Heatmaps can now be activated without loading data first
  • Thumbnail loading now has its own associated loading spinner and percent progress

Also, when loading data from the server I had a subtle bug, which is now fixed, where I was not specifying the field when doing Screen --> Plate --> Well --> WellSample --> Image queries. This resulted in potentially the wrong Image ID being put in the returned data and subsequent holes in the heatmaps.

We will have to be very careful with all of this going foward when we start bringing selectable fields into Parade. Especially if the field counts of Plates that are in a Screen differ.

I also had a lapse in logic where asynchronous calls for all thumbnails were being queued in the browser. On some of our test cases this could easily be >100. Exactly what I was previously afraid would happen did, promises stacked up and performance really suffered. The number of thumbnail batches that can load at one time is now limited to 6.

@will-moore
Copy link
Member

will-moore commented May 17, 2018

As discussed - there is probably too much here to try and get in to the first 0.0.1 release, so we'll exclude so we can test the release candidate build.

Note: J-M removed the exclude on 25/07

@jburel
Copy link
Member

jburel commented Jul 27, 2018

First round:

  • Settings>View: Dataset is always selected when viewing a plate/screen. Terminology is a bit confusing. It has no effect for S/P/ data
  • Heatmap color assigning: how is that determine? if all the values are the same it is black,
    if I have 3 values min is green, then middle black, max red. This could be a bit confusing
    If Mode>heatmap is selected, the user could select View>Thumbnails and gets confused since in that case nothing changes. This is obvious in List View.
  • I can see users wanting to keep option like View> Thumbnails selected when browsing plates. (similar to dataset).

@jburel
Copy link
Member

jburel commented Nov 16, 2018

@chris-allan what is the status of this PR?
#53 is conflicting with it. #53 is needed to fix a build failure

@chris-allan
Copy link
Member Author

@jburel: I haven't had time to get back to this fully based on the strategy of removing some of the more technically dangerous conditions that we discussed a while back. The contents of #53 are a bit troublesome with how much of the package lock file has changed.

Let's get together some time today to go over some options.

@jburel
Copy link
Member

jburel commented Nov 16, 2018

Discussed this am with @chris-allan. The plan is:

@will-moore will-moore mentioned this pull request Jul 19, 2019
@snoopycrimecop
Copy link
Member

Conflicting PR. Removed from build OMERO-plugins-push#26. See the console output for more details.
Possible conflicts:

  • Upstream changes
    • omero_parade/table_filters/data_providers.py
    • package-lock.json

--conflicts

@jburel
Copy link
Member

jburel commented Jul 19, 2019

--exclude

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.

4 participants