-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Tests fixes and contributor docs #952
Tests fixes and contributor docs #952
Conversation
…an enormous and complex traceback. see: vega/altair#972
…ake it a proper link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @Demetrio92! This type of PR can really help other users, I'm all for making it easier for people to contribute. Also good catch on the missing packages in requirements-dev
, though I couldn't find where two of them are used. I placed some comments, I hope you have time to reply.
.github/CONTRIBUTING.md
Outdated
5. Commit those changes | ||
5. Make sure the tests pass: | ||
* in the repository folder do `pip install -e .` (needed for notebook tests) | ||
* along with all the dependencies install `phantomjs` via `npm install -g phantomjs` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have npm, I think I got phantomjs from phantomjs.org. Maybe say the npm part as hint, or tell people how to get npm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see new commits
"\n", | ||
"try: \n", | ||
" from altair import Chart, load_dataset\n", | ||
"except TypeError:\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does altair raise a TypeError
on older versions? Seems weird. If altair really doesn't work on older Python version, it should raise a deprecation warning or exception. Can you tell what error you encountered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is what i told them...
vega/altair#972 (comment)
(check the link for the type of error I've gotten)
Apparently Python is partially responsible for the error, so it's complicated...
@@ -1,5 +1,6 @@ | |||
altair | |||
cartopy | |||
descartes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find descartes
, do you know where it was used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr it's b/c geopandas do no handle dependencies properly, but essentially it breaks
TestNotebooks.test_time_slider_choropleth
Full traceback
_______________________________________ TestNotebooks.test_time_slider_choropleth _______________________________________
self = <test_notebooks.NotebookTester object at 0x7f755a854e48>, exporter = None, filename = None
def __call__(self, exporter=None, filename=None):
raw_nb = nbconvert.exporters.Exporter().from_filename(self.filename)
raw_nb[0].metadata.setdefault('kernelspec', {})['name'] = 'python'
> exec_nb = nbconvert.preprocessors.ExecutePreprocessor(timeout=600).preprocess(*raw_nb)
tests/notebooks/test_notebooks.py:29:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
venv/lib/python3.5/site-packages/nbconvert/preprocessors/execute.py:262: in preprocess
nb, resources = super(ExecutePreprocessor, self).preprocess(nb, resources)
venv/lib/python3.5/site-packages/nbconvert/preprocessors/base.py:69: in preprocess
nb.cells[index], resources = self.preprocess_cell(cell, resources, index)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <nbconvert.preprocessors.execute.ExecutePreprocessor object at 0x7f7556331eb8>
cell = {'execution_count': 3, 'outputs': [{'ename': 'ImportError', 'output_type': 'error', 'evalue': 'The descartes package i...d': 'light'}}], 'source': '%matplotlib inline\n\nax = gdf.plot(figsize=(10, 10))', 'cell_type': 'code', 'metadata': {}}
resources = defaultdict(None, {'output_extension': '.txt', 'metadata': defaultdict(None, {'name': 'TimeSliderChoropleth', 'path': '/home/demetrio/Parkbob/git/folium/tests/notebooks/../../examples', 'modified_date': 'September 3, 2018'})})
cell_index = 3
def preprocess_cell(self, cell, resources, cell_index):
"""
Executes a single code cell. See base.py for details.
To execute all cells see :meth:`preprocess`.
"""
if cell.cell_type != 'code':
return cell, resources
reply, outputs = self.run_cell(cell, cell_index)
cell.outputs = outputs
if not self.allow_errors:
for out in outputs:
if out.output_type == 'error':
> raise CellExecutionError.from_cell_and_msg(cell, out)
E nbconvert.preprocessors.execute.CellExecutionError: An error occurred while executing the following cell:
E ------------------
E %matplotlib inline
E
E ax = gdf.plot(figsize=(10, 10))
E ------------------
E
E ---------------------------------------------------------------------------
E ImportError Traceback (most recent call last)
E ~/Parkbob/git/folium/venv/lib/python3.5/site-packages/geopandas/plotting.py in plot_polygon_collection(ax, geoms, values, color, cmap, vmin, vmax, **kwargs)
E 81 try:
E ---> 82 from descartes.patch import PolygonPatch
E 83 except ImportError:
E
E ImportError: No module named 'descartes'
E
E During handling of the above exception, another exception occurred:
E
E ImportError Traceback (most recent call last)
E <ipython-input-3-423c72b95e2e> in <module>()
E 1 get_ipython().run_line_magic('matplotlib', 'inline')
E 2
E ----> 3 ax = gdf.plot(figsize=(10, 10))
E
E ~/Parkbob/git/folium/venv/lib/python3.5/site-packages/geopandas/geodataframe.py in plot(self, *args, **kwargs)
E 532 from there.
E 533 """
E --> 534 return plot_dataframe(self, *args, **kwargs)
E 535
E 536 plot.__doc__ = plot_dataframe.__doc__
E
E ~/Parkbob/git/folium/venv/lib/python3.5/site-packages/geopandas/plotting.py in plot_dataframe(df, column, cmap, color, ax, categorical, legend, scheme, k, vmin, vmax, markersize, figsize, legend_kwds, **style_kwds)
E 412 return plot_series(df.geometry, cmap=cmap, color=color, ax=ax,
E 413 figsize=figsize, markersize=markersize,
E --> 414 **style_kwds)
E 415
E 416 # To accept pd.Series and np.arrays as column
E
E ~/Parkbob/git/folium/venv/lib/python3.5/site-packages/geopandas/plotting.py in plot_series(s, cmap, color, ax, figsize, **style_kwds)
E 292 values_ = values[poly_idx] if cmap else None
E 293 plot_polygon_collection(ax, polys, values_, facecolor=facecolor,
E --> 294 cmap=cmap, **style_kwds)
E 295
E 296 # plot all LineStrings and MultiLineString components in same collection
E
E ~/Parkbob/git/folium/venv/lib/python3.5/site-packages/geopandas/plotting.py in plot_polygon_collection(ax, geoms, values, color, cmap, vmin, vmax, **kwargs)
E 82 from descartes.patch import PolygonPatch
E 83 except ImportError:
E ---> 84 raise ImportError("The descartes package is required"
E 85 " for plotting polygons in geopandas.")
E 86 from matplotlib.collections import PatchCollection
E
E ImportError: The descartes package is required for plotting polygons in geopandas.
E ImportError: The descartes package is required for plotting polygons in geopandas.
venv/lib/python3.5/site-packages/nbconvert/preprocessors/execute.py:286: CellExecutionError
@@ -20,7 +22,9 @@ pillow | |||
pycodestyle | |||
pytest | |||
selenium | |||
scipy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for scipy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tl;dr it's b/c of cartopy not handling dependencies properly, but essentially it breaks
TestNotebooks.test_geodedetic_image_overlay
full traceback
______________________________________ TestNotebooks.test_geodedetic_image_overlay ______________________________________
self = <test_notebooks.NotebookTester object at 0x7f13471bfd30>, exporter = None, filename = None
def __call__(self, exporter=None, filename=None):
raw_nb = nbconvert.exporters.Exporter().from_filename(self.filename)
raw_nb[0].metadata.setdefault('kernelspec', {})['name'] = 'python'
> exec_nb = nbconvert.preprocessors.ExecutePreprocessor(timeout=600).preprocess(*raw_nb)
tests/notebooks/test_notebooks.py:29:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
venv/lib/python3.5/site-packages/nbconvert/preprocessors/execute.py:262: in preprocess
nb, resources = super(ExecutePreprocessor, self).preprocess(nb, resources)
venv/lib/python3.5/site-packages/nbconvert/preprocessors/base.py:69: in preprocess
nb.cells[index], resources = self.preprocess_cell(cell, resources, index)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <nbconvert.preprocessors.execute.ExecutePreprocessor object at 0x7f134398beb8>
cell = {'execution_count': 6, 'source': "import cartopy.crs as ccrs\nfrom cartopy.img_transform import warp_array\n\nsource_e... 'scipy'"], 'ename': 'ImportError', 'output_type': 'error', 'evalue': "No module named 'scipy'"}], 'cell_type': 'code'}
resources = defaultdict(None, {'metadata': defaultdict(None, {'name': 'GeodedeticImageOverlay', 'path': '/home/demetrio/Parkbob/git/folium/tests/notebooks/../../examples', 'modified_date': 'September 3, 2018'}), 'output_extension': '.txt'})
cell_index = 8
def preprocess_cell(self, cell, resources, cell_index):
"""
Executes a single code cell. See base.py for details.
To execute all cells see :meth:`preprocess`.
"""
if cell.cell_type != 'code':
return cell, resources
reply, outputs = self.run_cell(cell, cell_index)
cell.outputs = outputs
if not self.allow_errors:
for out in outputs:
if out.output_type == 'error':
> raise CellExecutionError.from_cell_and_msg(cell, out)
E nbconvert.preprocessors.execute.CellExecutionError: An error occurred while executing the following cell:
E ------------------
E import cartopy.crs as ccrs
E from cartopy.img_transform import warp_array
E
E source_extent = [lon.min(), lon.max(), lat.min(), lat.max()]
E
E new_data = warp_array(colored_data,
E target_proj=ccrs.GOOGLE_MERCATOR,
E source_proj=ccrs.PlateCarree(),
E target_res=data.shape,
E source_extent=source_extent,
E target_extent=None,
E mask_extrapolated=False)
E
E
E m = folium.Map(location=[lat.mean(), lon.mean()], zoom_start=1)
E
E folium.raster_layers.ImageOverlay(
E image=new_data[0],
E bounds=[[lat.min(), lon.min()], [lat.max(), lon.max()]],
E opacity=0.25
E ).add_to(m)
E
E m.save(os.path.join('results', 'GeodedeticImageOverlay_2.html'))
E
E m
E ------------------
E
E ---------------------------------------------------------------------------
E ImportError Traceback (most recent call last)
E <ipython-input-6-f0b9096a59bd> in <module>()
E 1 import cartopy.crs as ccrs
E ----> 2 from cartopy.img_transform import warp_array
E 3
E 4 source_extent = [lon.min(), lon.max(), lat.min(), lat.max()]
E 5
E
E ~/Parkbob/git/folium/venv/lib/python3.5/site-packages/cartopy/img_transform.py in <module>()
E 24
E 25 import numpy as np
E ---> 26 import scipy.spatial
E 27
E 28 import cartopy.crs as ccrs
E
E ImportError: No module named 'scipy'
E ImportError: No module named 'scipy'
venv/lib/python3.5/site-packages/nbconvert/preprocessors/execute.py:286: CellExecutionError
they already know that: SciTools/cartopy#1112
README.rst
Outdated
Contributing | ||
------------ | ||
|
||
see `.github/CONTRIBUTING.md` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add one or two lines here so it's a bit more welcoming. Something about how folium is maintained by volunteers, that contributions are welcome, and that people should check the issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, i will, you're right!
Can you help me with the link? I tried .. _relative_link
but it seems to only work as rst-to-rst :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see new commits
@Conengmo i guess we have a small timezone issue here. Please let both of my PRs sit here for a few days, i will address all the comments |
I think I addressed all the issues, let me know if anything still remains. |
Complete link to the contributors guide. Also updated the documentation version.
Screwed up the documentation links :(
Thanks for helping out @Demetrio92! |
While I was working on another PR, I noticed that few things are missing in the repository so a contributor is able to set up the dev enviroment. I did cost me quite some time to figure them all out, so here is a PR collecting them all.