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

[ENH] Map widget #1735

Merged
merged 38 commits into from
Jan 16, 2017
Merged

[ENH] Map widget #1735

merged 38 commits into from
Jan 16, 2017

Conversation

kernc
Copy link
Contributor

@kernc kernc commented Nov 7, 2016

Issue

Add map widget for plotting geospatial data.

Includes
  • Code changes
  • Tests
  • Documentation

@astaric
Copy link
Member

astaric commented Nov 18, 2016

Can you skip the owmap tests when only PyQt4 is available? As we will not drop support for PyQt4 for some time, it would be nice if the tests still passed.

@lanzagar lanzagar added this to the 3.3.9 milestone Nov 25, 2016
Copy link
Contributor

@lanzagar lanzagar left a comment

Choose a reason for hiding this comment

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

  1. Selections on an empty widget result in a crash.
  2. Zoom to rectangle selection does not work for me.
  3. Tooltip has duplicates if e.g. shape and color use the same var.
  4. All other widget windows get closed with Esc on my system except for this one (anything different/non-standard here?)
  5. When target is selected an error is shown if no learner. If learner is connected and target selected, removing the learner does not show the error (until deselect/select of target var).

Non-urgent wishes (for later PRs; to be discussed):

  • This is very similar to Scatter plot so we should strive for similar controls (order of opacity/symbol size; location of jittering; zoom/select controls; etc)
  • Also, scatter plot has class density which could be very relevant here as well.
    Maybe even more than the included heatmap feature. Even better, we should expand class density to support continuous vars, allow target selection, and include that in both widgets.

@ajdapretnar
Copy link
Contributor

Documentation is prepared but waiting for some additional fixes (heatmap for classification and resolution regarding the test data set).

I am experiencing the same issues as @lanzagar with additional issue of labeling by name on airports data set. I get AttributeError: 'LeafletMap' object has no attribute '_label_values'. In essence, labeling by string doesn't work for me.

@kernc kernc force-pushed the owmap branch 6 times, most recently from 43c9906 to 4d21b34 Compare December 1, 2016 15:47
@kernc
Copy link
Contributor Author

kernc commented Dec 1, 2016

@ajdapretnar Have yet to confirm, but there may not be a classification heatmap so soon.

Zoom to rectangle selection does not work for me.

@lanzagar Any errors in the error console?

@lanzagar lanzagar modified the milestones: 3.3.10, 3.3.9 Dec 2, 2016
@lanzagar
Copy link
Contributor

lanzagar commented Dec 2, 2016

Bumping this to 3.3.10 as it is still under active development and needs documentation etc.

@codecov-io
Copy link

codecov-io commented Dec 2, 2016

Current coverage is 89.27% (diff: 50.00%)

Merging #1735 into master will decrease coverage by 0.01%

@@             master      #1735   diff @@
==========================================
  Files            86         86          
  Lines          9112       9116     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           8136       8138     +2   
- Misses          976        978     +2   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update 4dad654...a83e3ee

@kernc
Copy link
Contributor Author

kernc commented Dec 2, 2016

@ajdapretnar Have yet to confirm, but there may not be a classification heatmap so soon.

Managed something.

screenshot - 12022016 - 11 34 23 am

@ajdapretnar
Copy link
Contributor

That's exactly what I had in mind. Thanks! :) Will test it again next week!

@kernc
Copy link
Contributor Author

kernc commented Dec 2, 2016

Blocked on #1799.

@kernc kernc force-pushed the owmap branch 3 times, most recently from 5b33dbe to 4bacffe Compare December 5, 2016 17:20
Copy link
Contributor

@lanzagar lanzagar left a comment

Choose a reason for hiding this comment

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

Previous issues seem to be fixed.
Some new ones include:

  • Changing lat/long var does not update map!
  • Changing from a discrete to continuous target var for overlay has a bug
  • Starting zoom is not correct (clicking the center button after that works)

Copy link
Contributor

@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.

I wholeheartedly approve this widget. MASAP.

Copy link
Contributor

@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.

Bumping the approval count.

@kernc kernc changed the title [WIP] [ENH] Map widget [ENH] Map widget Jan 13, 2017
@lanzagar lanzagar merged commit 463242d into biolab:master Jan 16, 2017
@astaric astaric modified the milestone: future Apr 19, 2017
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.

8 participants