Added venv/kernel instructions to README #3
Conversation
|
Added additional commits to update grain method description. |
|
Completed sample data creation notebook, removed all data files from repo since notebook downloads counties data and creates county subset data, updated grain_method description in tutorial, updated warning message for exceeding max cache limit, and updated shapefile zipping. |
…aArray objects of different variables from the same dataset into an xarray.DataSet.
HeatherSavoy-USDA
left a comment
HeatherSavoy-USDA
left a comment
There was a problem hiding this comment.
Some of my comments are general and don't need action now, but they could be made into new issues so that they will be documented for addressing later.
requirements.txt
Outdated
There was a problem hiding this comment.
I don't know if we want ipykernel as a dependency. Does ipykernel have a use outside of setting up kernels for the notebooks? Is it typical to include a dependency like this in a Python package when it isn't needed for the functionality of the package?
There was a problem hiding this comment.
I added this here because to in the readme, we explain how to set up a jupyter notebook for pygcdl on ceres, and we instruct users to call both pip install -r requirements.txt and pip install ipykernel. I added ipykernel to requirements.txt to avoid having to call pip install twice, but then accidentally left the pip install ipykernel command in the readme. I agree that since it is not needed for the functionality of the package on its own, it shouldn't be included. Updated on fork.
Layer_merging_bug.ipynb
Outdated
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "pygcdl_obj = pygcdl.PyGeoCDL(url_base=\"http://127.0.0.1:8000\")" |
There was a problem hiding this comment.
I think we should avoid committing assumptions of local testing.
There was a problem hiding this comment.
Removed file on fork, since the underlying gcdl issue is already documented.
Layer_merging_bug.ipynb
Outdated
| "import pygcdl\n", | ||
| "import geopandas as gpd\n", | ||
| "import rioxarray\n", | ||
| "import matplotlib.pyplot as plt\n", |
There was a problem hiding this comment.
It would be nice if examples used plotnine for visualizations. This is more relevant to tutorial development, though, and not so much for package development.
There was a problem hiding this comment.
Noted. The pygcdl_tutorial.ipynb tutorial uses matplotlib for visualizations. I can work on switching that over to plotnine.
There was a problem hiding this comment.
I know it is common to have some example data included in a R package, but I am less familiar with Python - is that a thing that is done in Python packages? And would the example data files be expected to be provided, i.e., not as instructions for downloading?
In general, I'm curious to see how all of these notebooks will evolve into documentation for the structured package.
There was a problem hiding this comment.
I agree that this needs to be better organized. Currently, the example data here is used for testing only, and is not needed on the user's end. Should I put all of the files that are exclusively for testing purposes and not needed by the users in a designated "testing" directory to keep it separate? I am currently doing testing informally in a notebook, but will switch over to using pytest.
pygcdl.py
Outdated
There was a problem hiding this comment.
Should this commented out line be deleted? See line 367 above as well.
There was a problem hiding this comment.
Fixed on fork
pygcdl.py
Outdated
There was a problem hiding this comment.
Is this print statement intentional or leftover from debugging?
There was a problem hiding this comment.
Fixed on fork
README.md
Outdated
|
|
||
| Python interface for the Geospatial Common Data Library. | ||
|
|
||
| To set up a jupyter notebook kernel for pygcdl, run the following commands: |
There was a problem hiding this comment.
Fixed on fork
README.md
Outdated
|
|
||
| To set up a jupyter notebook kernel for pygcdl, run the following commands: | ||
|
|
||
| module load python_3 |
There was a problem hiding this comment.
I think you might need to make a code block in markdown for this section to look nicely rendered.
There was a problem hiding this comment.
Fixed on fork
There was a problem hiding this comment.
Approving for now assuming everything will be translated to formal testing later.
…ing, updated readme.
…taset_info(), upload_geometry(), and download_polygon_subset().
2 participants
No description provided.