Tests and documentation for rose.plot() and rose.plot_vectors()
#47
Conversation
rose.plot() and rose.plot_vectors()rose.plot() and rose.plot_vectors()
and allow for `arrows` argument in `.plot_vectors()`
update documentation no coloring option `attribute` in `.plot_vectors()`
| warnings.warn('This method relies on importing `splot` in future', | ||
| DeprecationWarning) | ||
|
|
||
| if use_splot: |
There was a problem hiding this comment.
if the import fails, then you'll see the warning in line 388, and then this will fail on a name error.
This happens because use_splot isn't defined by the time you hit this if.
I think you can solve this if you set use_splot=False after line 389
There was a problem hiding this comment.
I am not sure about the current model of splot in terms of supporting visualization for other pysal packages such as giddy and esda. My thought was that giddy would only focus on analytics while leaving visualization to splot, meaning that giddy would not have a plot function but users can easily import splot when visualization is needed. We can also demonstrate the visualization functionalities provided by splot in jupyter notebook gallery. (splot and geopandas will be included in the requirement_dev.txt to support sphinx building) In this way, giddy will be a lightweight package.
There was a problem hiding this comment.
@weikang9009 the idea was that the plotting functionality remains in splot and can be called as a .plot() method inside other sub packages. This makes plotting specific objects more user friendly. I will change the import/ install requirement of splot to make it a soft-dependency so giddy remains a lightweight package with a plotting option if needed.
There was a problem hiding this comment.
I apologize to @weikang9009 as I didn't convey the splot based discussions that led to the this pr and its potential implications for giddy.
Not sure this creates any weight for giddy as splot is treated as a soft dependency here.
I am +1 on adding the plot method as I think it makes sense for the designers of giddy to suggest the default analytical plots. Again, it is based on a soft dependency so we don't increase the install requirements, but give the interested user warnings if they try the plot method without splot installed.
There was a problem hiding this comment.
Thanks for the clarification.
| warnings.warn('This method relies on importing `splot` in future', | ||
| DeprecationWarning) | ||
|
|
||
| if use_splot: |
| fig, _ = rose.plot(attribute=y1) | ||
| plt.close(fig) | ||
|
|
||
|
|
There was a problem hiding this comment.
I think these need to use an @unittest.skipIf decorator, unless @weikang9009 and @sjsrey are fine with adding geopandas as a development dependency for giddy.
I think it's fine, but I'm not a giddy maintainer 😄
| - conda install --yes pip | ||
| - conda install --yes nose | ||
| - pip install -r requirements.txt | ||
| - pip install libpysal esda mapclassify |
There was a problem hiding this comment.
libpysal, esda and mapclassify are included in requirements.txt now
| warnings.warn('This method relies on importing `splot` in future', | ||
| DeprecationWarning) | ||
|
|
||
| if use_splot: |
There was a problem hiding this comment.
I am not sure about the current model of splot in terms of supporting visualization for other pysal packages such as giddy and esda. My thought was that giddy would only focus on analytics while leaving visualization to splot, meaning that giddy would not have a plot function but users can easily import splot when visualization is needed. We can also demonstrate the visualization functionalities provided by splot in jupyter notebook gallery. (splot and geopandas will be included in the requirement_dev.txt to support sphinx building) In this way, giddy will be a lightweight package.
test and documentation for
rose.plot()androse.plot_vectors()added tests
added documentation
added DeprecationWarning
added
splotinstall to.travis.yml