Skip to content

Conversation

@robertcv
Copy link
Collaborator

@robertcv robertcv commented Feb 4, 2020

Issue

The old OWChoropleth widget is based on a javascript library and is running in a WebviewWidget. As a result, it has a lot of hard to debug problems.

Description of changes

Implemented an OWScatterPlotBase like plot class for dawing choropleth and maps. Update widget style to a more modern look.

This is a working version for testing. Bugs may occur. Open to suggestions for small enhancements.

TODOs:
  • rebase after [ENH] Move map plot utils to separate file #91 is merged
  • number of color buckets
  • fix discret colors to be the same as agg_attr -> scatterplot doesn't support it
  • add caching
  • add settings migration
  • fix tooltip
  • add selection saving
  • add support for time data
Includes
  • Code changes
  • Tests
  • Documentation

@robertcv
Copy link
Collaborator Author

robertcv commented Feb 4, 2020

@ajdapretnar @AndrejaKovacic take a look :)

@ajdapretnar
Copy link
Collaborator

I propose switching automatically to 'mode' when discrete attribute is selected for the first time. And disabling all aggregations that don't apply.

@AndrejaKovacic
Copy link
Collaborator

Tooltip is nice, but i think it should also include the exact values of what is being plotted for the region.
E.g.: Sum (attribute): 3456

@codecov-io
Copy link

codecov-io commented Feb 7, 2020

Codecov Report

Merging #89 into master will increase coverage by 15.42%.
The diff coverage is 81.12%.

@@             Coverage Diff             @@
##           master      #89       +/-   ##
===========================================
+ Coverage   68.33%   83.75%   +15.42%     
===========================================
  Files           9        9               
  Lines        1197     1607      +410     
  Branches      168      236       +68     
===========================================
+ Hits          818     1346      +528     
+ Misses        341      200      -141     
- Partials       38       61       +23

@robertcv
Copy link
Collaborator Author

This is now incompatible with old releases of Orange due to the changes of color palettes on Orange master.

@robertcv robertcv requested a review from janezd February 24, 2020 10:03
@janezd
Copy link
Collaborator

janezd commented Mar 3, 2020

  1. Connect File -> Choropleth.
  2. Select some regions.
  3. Connect Choropleth to another Choropleth. Column "Selected" does not appear because you connected "Selected data", not "Data"
  4. Double click the connection and connect "Data" instead. Column Selected still doesn't appear.
  5. Ask @VesnaT how to fix this. Perhaps see Scatter plot does not detect coloring changes orange3#4391 and [FIX] DataProjectionWidget: Update combos on new data orange3#4405, if this is a similar problem.

@janezd
Copy link
Collaborator

janezd commented Mar 3, 2020

Would it be possible to put names above colored polygons?

Screenshot 2020-03-03 at 19 00 04

Copy link
Collaborator

@janezd janezd left a comment

Choose a reason for hiding this comment

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

Nice.

I've clicked around and experienced no crashes. But perhaps this widget needs to be thouroughly clicked through by somebody who already used it and know practical use cases.

I reviewed the code, except the parts related to maps, about which I don't know much. I/We will have to trust you on this.

It would be great if you added more tests, but I won't object if you don't.

@robertcv
Copy link
Collaborator Author

robertcv commented Mar 5, 2020

Would it be possible to put names above colored polygons?

While possible to show maps without names and overlaying them on top of polygons, it is a lot of work. I wouldn't implement it in this pr.

@janezd
Copy link
Collaborator

janezd commented Mar 5, 2020

Would it be possible to put names above colored polygons?

While possible to show maps without names and overlaying them on top of polygons, it is a lot of work. I wouldn't implement it in this pr.

OK, understood.

@robertcv robertcv force-pushed the enh/new_choropleth branch from f3fcb69 to 0767f96 Compare March 11, 2020 11:22
@robertcv
Copy link
Collaborator Author

@janezd I changed the attr-agg handling to reflect the suggested changes. This should now be the final version if no additional issues are found. Now I just don't know what to do with new pallets since there is no new Orange release. Perhaps this can be merged but then we withe for new geo addon release.

@robertcv robertcv changed the title [WIP][ENH] Reimplementation of OWChoropleth [ENH] Reimplementation of OWChoropleth Mar 11, 2020
@janezd
Copy link
Collaborator

janezd commented Mar 12, 2020

I think you can merge this, but wait with the release.

@robertcv robertcv force-pushed the enh/new_choropleth branch from 0457291 to 6d83c9c Compare April 1, 2020 12:51
@janezd
Copy link
Collaborator

janezd commented Apr 2, 2020

κῦδος for this monumental work!

I'm merging it now, but the release has to wait for the release of Orange.

@janezd janezd merged commit 4f9c4e8 into biolab:master Apr 2, 2020
@janezd janezd mentioned this pull request Apr 2, 2020
@robertcv robertcv deleted the enh/new_choropleth branch April 9, 2020 14:07
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.

6 participants