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

Add label properties app #518

Merged
merged 27 commits into from Nov 11, 2020
Merged

Add label properties app #518

merged 27 commits into from Nov 11, 2020

Conversation

surchs
Copy link
Contributor

@surchs surchs commented Oct 28, 2020

Issue for app: #507

App pull request

  • This is a new app
  • I am improving an existing app (redesigns/code "makeovers")

About

Workflow

  • I have created a branch in the appropriate monorepo, and the
    elements necessary for successful deployment are in place.
  • If the app is a redesigned and/or restyled version of an
    existing gallery app, I've summarized the changes requested in the
    appropriate Streambed issue and confirm that they have been applied.
  • If the app is on the Dash Gallery portal, I have added a link to
    the GitHub repository for the source code in the portal description.
  • If the app is a reimplementation of a Python gallery app for the
    DashR gallery, the app in this PR mimics, as closely as possible,
    the style and functionality of the existing app.=
  • I have removed all Google Analytics code from the app's
    assets/ folder.

The pre-review review

I have addressed all of the following questions:

  • Does everything in my code serve some purpose? (I have removed
    any dead and/or irrelevant code.)
  • Does everything in my code have a clear purpose? (My code is
    readable and, where it isn't, it has been commented appropriately.)]
  • Am I reinventing the wheel? (I have used appropriate packages to
    lessen the volume of code that needs to be maintained.)

- copied [object-properties](https://github.com/plotly/canvas-portal/blob/master/apps/object-properties/app.py) app from canvas-portal
- changed dash_table calls to work with updated API
- used px.imshow to visualize the label data
- if callback inputs were not triggered by user interaction (i.e. when the app is just loaded), just return default values
- removed the suppression of callback exceptions
- added dbc to requirements.txt
- added dbc stylesheet to app
- remove old stylesheets that are no longer needed
- moved app and content definition to the top of the file
- refactored `data` to `table` to avoid shadowed variable name from `highlight_filter` scope and for clarity
- broke layout into components and fit in dbc structure with annotations
- added plotly logo
- added a README.md file
@surchs
Copy link
Contributor Author

surchs commented Oct 28, 2020

This PR isn't completely finished yet. For example, the app currently doesn't show very interesting hover information as discussed in #507. I wanted to start the review process at this stage already because there are a couple of changes that I'd like feedback on.

surchs and others added 4 commits October 28, 2020 19:29
- my linter didn't catch that...
change to new app name
Copy link
Contributor

@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left a couple of comments. Also, some columns of the table are a bit too wide, it would be possible to change columns headers (euler_number --> "Euler number") to fit on two lines to have less wide columns. This would make it possible to have a wider column for the figure. What would you think of this?

line=dict(width=1),
showscale=False,
colorscale=custom_viridis,
opacity=opacity,
Copy link
Contributor

Choose a reason for hiding this comment

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

a hovertemplate could be added here to display only the z value (not x and y) and to add for example the mean intensity (passed as customdata, see for example https://plotly.com/python/hover-text-and-formatting/#adding-other-data-to-the-hover-with-customdata-and-a-hovertemplate)

apps/dash-label-properties/app.py Show resolved Hide resolved
apps/dash-label-properties/app.py Outdated Show resolved Hide resolved
apps/dash-label-properties/app.py Outdated Show resolved Hide resolved
apps/dash-label-properties/app.py Show resolved Hide resolved
apps/dash-label-properties/app.py Show resolved Hide resolved
- use new skimage region property computation function to construct data-table
- prevent call to `highlight_row` and `highlight_filter` callbacks at app start.
- pass data-table to image function
- match custom hover info with the currently displayed data
- also remove margins around figure
# Display hover data with precision if data is float
hover_string = [
f"{col}: %{{customdata[{col_id}]:.3f}}"
if data_table[col].dtype in (np.float,)
Copy link
Contributor Author

@surchs surchs Oct 30, 2020

Choose a reason for hiding this comment

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

I am not entirely sure why this line works. For example data_table[col].dtype in ("float",) has the same result which is not encouraging. I think it is somewhat explicit but maybe there is a better way to write this? I don't think I have access to the precision from the pandas object.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is basically what np.array_str does (but it adds these [...] things around the numbers), so it sort of seems the way to do it. The arrays I think are just numpy arrays underneath. I think it's ok that data_table[col].dtype in "float" float has the same result, because in numpy you can make an array like np.zeros(N,dtype='float') or np.zeros(N,dtype=np.float) and they will have the same type. That it doesn't check that it is one of those two types is a another story involving Python's type checking system in general... but I think we don't have to worry about that (we provided the data, haha).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the pointers @nicholas-esterer! I dug a little further because I didn't understand why np.dtype("float") == "float" evaluates True essentially.
The way I understand it now is that 1) numpy datatypes have a hierarchy where np.float64 is a sub-datatype of float and 2) numpy array's have a __eq__ method that evaluates this hierarchy internally. So you can also do things like np.dtype("float") == "double" and so on...
Guess I learned something new today!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think to be super explicit here, I'll just go with np.issubdtype here to avoid any confusion

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you can get away with just using printf statements, does this do what you want?

>>> '%0.3f' % (1.23,)
'1.230'
>>> '%0.3f' % (1,)
'1.000'

1 is an int but has 0s after it, 1.23 is made to have an extra 0

@nicholas-esterer
Copy link
Contributor

Sometimes I get the following error message:

  File "/home/nicke/development/dash-sample-apps/apps/dash-label-properties/app.py", line 299, in higlight_row
    "if": {"filter_query": "{label} eq %d" % index},
TypeError: %d format: a number is required, not dict

(there's more in the traceback but it's not in your app anymore)
For some reason index is sometimes a dict? I can't find what triggered this though, sorry!

@surchs
Copy link
Contributor Author

surchs commented Oct 30, 2020

Sometimes I get the following error message:

  File "/home/nicke/development/dash-sample-apps/apps/dash-label-properties/app.py", line 299, in higlight_row
    "if": {"filter_query": "{label} eq %d" % index},
TypeError: %d format: a number is required, not dict

Hhm, I also remember seeing that but I thought I had debugged this and haven't seen it since. Does that usually happen right at the start of the app? I think that might have been taken care off with the prevent_initial_call=True but I am not sure. I'll see if I can make it come up again.

(there's more in the traceback but it's not in your app anymore)
For some reason index is sometimes a dict? I can't find what triggered this though, sorry!

I don't think I understand what this means. Have I removed something that makes the traceback more informative?

@nicholas-esterer
Copy link
Contributor

No no, it's just I can't always trigger it, and I didn't look at the problem that long (I had hoped I could show you better where the bug was). Maybe just try putting
print(index)
just before and see what it shows.

@surchs
Copy link
Contributor Author

surchs commented Oct 30, 2020

Ah ok, cool, thanks for that. I have definitely also seen this but without remembering the specific fix, I thought I had taken care of it. Would be curious to see what brings it up.

- more explicit numpy datatype checking for the datatable columns
- added 1px margins (l/r) around .row elements to have datatable display fully
- added list of unique labels as cache
- removed label array Store and update from callback
- replaced functionality with list of currently visible labels
- some label related refactoring for clarity
surchs and others added 8 commits November 2, 2020 12:49
- converted label overlay to png using datashader
- added datashader to requirements.txt
- additional hidden scatter overlay for hoverinfo
- adjusted highlight_row callback to work with the scatter overlay
- added customdata with label information
- required to build datashader dependency
@surchs
Copy link
Contributor Author

surchs commented Nov 6, 2020

I have encountered a bit of a tricky issue: I have added datashader to the requirements to convert my 2D numpy array to a 3-channel RGB image. datashader depends on numba which depends on llvmlite which in turn depends on llvm. Now datashader is pretty up to date in terms of its requirements and the current version requires llvm>=9 to install. Our CircleCI container is running Debian Stretch which offers up to llvm-7 (including backports). There is a nightly repo for llvm that provide recent versions and I can successfully install and configure it in the CI. However, even with this set up, installing llvmlite eventually fails because it can't find SVML.

I don't really understand why this is proving so difficult. After all there is a data-shader app in this repo and the config doesn't appear to be radically different (other than pinning python 3.6). But bottom line: I can't manage to get the app with datashader deployed through the CI. Maybe @nicholas-esterer or @emmanuelle, you have any ideas?

edit:
I guess one solution would be to pin datashader to some older version that can live with the old llvm we have available?

@emmanuelle
Copy link
Contributor

@jonmmease maybe you can help @surchs with the deployment problem coming from datashader's dependencies.

@surchs if this is blocking you can probably vendor datashader's module together with the app, keeping only what you need and removing the numba decorator of https://github.com/holoviz/datashader/blob/master/datashader/transfer_functions/__init__.py

Before this you can try to also pin numba to an older version explicitly, probably pinning only datashader is not enough.

@jonmmease
Copy link

I touched base with @surchs offline a bit on Friday. Here are a set of versions that currently work with the old version of pip that is included in the DE buildpack (https://github.com/plotly/dash-world-cell-towers/blob/d02ebf370b6f31196386a1939dc5e3ac3fe71317/requirements.txt).

After pip is updated (plotly/heroku-buildpack-python#38), these errors should all go away.

@surchs
Copy link
Contributor Author

surchs commented Nov 9, 2020

Thanks @jonmmease for the pointers to the working versions and @emmanuelle for the ping. I will try these out with the next commit!

- removed hoverinfo for background image
- made the content of the hover dependent on the selected columns in the datatable
- remove current label cache. Is not necessary because relevant information is available from table data
- fixed bug where contour was drawn based on row index rather than label.
- updated requirements.txt to pin CircleCI compatible versions (thanks @jonmmease)
- removed typecheck in image_with_contour since we supply the PIL objects
- dropdown to control what the color overlay is based on
- css change to give Cards margins on top
@surchs
Copy link
Contributor Author

surchs commented Nov 10, 2020

I believe that the core functionality is there. Remaining issues:

  • [CANT FIX] colorbar is not consistent through filter events because datashader.shade doesn't implement span correctly in this version (to manually set the value range)
  • [HELP WANTED] the hidden (opaque) scatter traces used to get the hoverinfo somehow add margins to the canvas that look annoying and also disappear when user zooms in. Maybe there is some elegant way to solve this @nicholas-esterer ?
  • [WILL FIX] we should probably update the modal with the new functionality and new screenshot.

@jonmmease
Copy link

[CANT FIX] colorbar is not consistent through filter events because datashader.shade doesn't implement span correctly in this version (to manually set the value range)

FWIW, span doesn't work with the default shade method (shade(..., how=eq_hist)), but it should work with the other methods (linear, log, and cbrt) unless there was a bug in this version of datashader. These won't give the same even distribution of colors, but you may want to try them out to get span support.

Code: https://datashader.org/_modules/datashader/transfer_functions.html#shade

@emmanuelle
Copy link
Contributor

emmanuelle commented Nov 10, 2020

Thank you @surchs I really love the app! The dropdown with the different ways of coloring the objects is very nice. One thing I noticed is that if you want to select an object in the table (the red outline) you need to select a value in the dropdown first or it won't work. I did not investigate why, I can take a look unless it's obvious for you.

Also could you please explain

[HELP WANTED] the hidden (opaque) scatter traces used to get the hoverinfo somehow add margins to the canvas that look annoying and also disappear when user zooms in. Maybe there is some elegant way to solve this @nicholas-esterer ?
I did not notice annoying margins.

Last thing (before I read the code, I've just tested the deployed app for now): I'm not sure I understand what the checkboxes in the columns headers are for (maybe it's this cell selection thing I was talking about and I was not clear about what I wanted? Anyway for now the app has all the features I asked for, but I don't know how to use these checkboxes). (Edited: I understood after reading previous comments, maybe a tooltip or a line of text somewhere could explain what you can do when selecting columns).

And yes please write one of these great modals you've been treating us with, with a screenshot of the much-improved layout :-).

@@ -0,0 +1,16 @@
datashader==0.8.0
dash-core-components
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that when you put dash in the requirements.txt, or pip install dash, you got dash-core-components, dash-html-components and dash-table together with dash. Am I missing something? (maybe it's related to the datashader problems?) I'm asking because we've been trying to tell users (on the forum for example) to install dash only and not to separately install dcc and html because they would run into version incompatibilities.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may have just copied this over from an existing requirements.txt. I'll make sure we don't need it and then trim the requirements!

Copy link
Contributor Author

@surchs surchs Nov 10, 2020

Choose a reason for hiding this comment

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

I looked at pipdeptree and all we really need is dash, dash_canvas and dash_bootstrap_components (now that we got rid of the datashader dependencies) edit: we also need explicitly pandas and gunicorn. For visibility purposes it might make sense to add skimage back in, what do you think @emmanuelle

@@ -0,0 +1,9 @@
/* set the margins of row elements to zero to
override the settings in the dash bootstrap stylesheet */
.row {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love it when the CSS stylesheet has a length of 9 lines :-).

@emmanuelle
Copy link
Contributor

ok I saw the margins
image

@emmanuelle
Copy link
Contributor

For the margins a quick fix would probably to use a different template such as simple_white which has a white background and no grid? https://plotly.com/python/templates/ There might be a more elegant solution of course.

@surchs
Copy link
Contributor Author

surchs commented Nov 10, 2020

Also could you please explain

[HELP WANTED] the hidden (opaque) scatter traces used to get the hoverinfo somehow add margins to the canvas that look annoying and also disappear when user zooms in. Maybe there is some elegant way to solve this @nicholas-esterer ?
I did not notice annoying margins.

image
I mean these "margins" at the top and bottom of the figure. If you zoom in, they disappear:
image

They are related to the hidden scatter trace that the hover-info is based on (i.e. if the traces are not added, the figure fills the entire canvas). I assume that the scatter traces add their own x and y range "padding" and therefore extend beyond the base image range. So far I haven't found a good way to keep them from doing that. Maybe I should just manually compute the max/min of the positions and set the ranges to that?

Last thing (before I read the code, I've just tested the deployed app for now): I'm not sure I understand what the checkboxes in the columns headers are for (maybe it's this cell selection thing I was talking about and I was not clear about what I wanted? Anyway for now the app has all the features I asked for, but I don't know how to use these checkboxes).

Yeah, I need to explain this :). I was thinking what we could do with the selected_columns feature and so at the moment it controls the information that is shown in the hover-info. That is, you only see the variables from the selected columns (because by default I showed all and that gets a bit busy). I tried to use a datatable tooltip to explain this but I think the version we use here doesn't have that feature yet? The description could either go in the modal or on top of the table inside the card.

And yes please write one of these great modals you've been treating us with, with a screenshot of the much-improved layout :-).

Will do :)



def image_with_contour(
img, active_labels, data_table, active_columns, color_column, mode="lines"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this app mode is always None so probably you can simplify the code a bit here?


fig = px.imshow(img, binary_string=True, binary_backend="jpg",)
# Overlay the current labels
_, hex_list = zip(*colors.PLOTLY_SCALES["Viridis"])
Copy link
Contributor

Choose a reason for hiding this comment

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

can you wrap this part (the shading) in a helper function? It will be clearer and it's also something we could reuse in other apps so it will be easier to find it back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we no longer need this code/function in the label-properties app. Do you think we should still wrap it into a helper function and if so, what would be a good place to put it?

Copy link
Contributor

Choose a reason for hiding this comment

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

please remove any code that is not needed any more. In order to keep track of what you did here maybe we could add an example to the imshow page of the plotly doc (on how to pass grayscale image as a binary image string but still use a colormap) but this is quite a hack so maybe we can just remember that this is possible and we'll find the code in the PR if needed?

):
"""
Figure with contour plot of labels superimposed on background image.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this function is quite advanced, would it be possible to give a few explanations in the docstring about the different traces which are used, and the tricks to improve performance?

)
fig.add_scatter(
x=x,
y=y,
Copy link
Contributor

Choose a reason for hiding this comment

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

is the layout Image above really needed here, or would it be possible just to use the filled scatter traces with suitable colors computed from with datashader? Not very important since it works as it is but I'm just trying to see whether we can reduce the complexity of the function or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that's a great idea. And I think we don't even need the datashader then anymore because we can simply use the matplotlib cmap object for "viridis" and call the rgb colors for each (normalized) value we want to draw individually. I have committed this and I think it's a lot cleaner and most of the complexity is gone.

Unfortunately I don't know how to integrate the colorbar-scatter into the "color overlay" scatter. So for the moment, we have these two, relatively lengthy scatter traces in there.

@emmanuelle
Copy link
Contributor

OK I finished my review!

surchs and others added 6 commits November 10, 2020 11:27
- scatter trace is now opaque and gets colored based on the selected value. px.imshow color overlay is removed
- replaced datashader with matplotlib cmap call
- removed datashader and dependencies from requirements.txt
- trimmed requirements.txt to minimal requirements according to pipdeptree
- changed figure template to "simple_white" to hide the added margins, made axes invisible
- added initial value to dropdown menu that prevented the region contour from drawing correctly on app start (or before a dropdown value was explicitly set by the user)
- increased the line thickness of the region contour to stand out more against the colored scatter trace
incorrectly removed the two dependencies
- added new modal and screenshot / gif
- added warning popup if colorscale is deselected (breaks things)
- pointed github button to correct repo
- added tooltips to datatable to better explain column selection
- annotated the image generator function
- cleaned up the imports
- screenshot link typo fixed
- added stylesheet rule to keep screenshot from overflowing modal
Copy link
Contributor

@emmanuelle emmanuelle left a comment

Choose a reason for hiding this comment

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

Awesome, thank you @surchs! 💃

@surchs surchs merged commit 36ecd96 into master Nov 11, 2020
@surchs surchs deleted the add-label-properties-app branch November 11, 2020 14:48
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.

None yet

4 participants