-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix several issues in geoviewer #418
Conversation
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.
Looking good, thanks for diving into this!
For documenting: I would add a "command line user guide" like in Rasterio (but in the Documentation section, not as its own header): https://rasterio.readthedocs.io/en/stable/cli.html
For test data: We could add a script in examples.py
that creates artificial 2D/4D data, or as a pytest fixture.
So you mean, I should have a subsection in the "Reference" section, e.g. after the API?
Right, yes we could have this in the pytest fixture, as it does not make much sense to store the artificial data. |
Yes, exactly! A To automatically document the function, looks like we can add the |
I've managed to build the CLI documentation locally, but it fails on readthedocs (will be visible at this link). One issue is that the usage window is too large for the width so we need to scroll to see the whole usage. I tried the solutions given at this link without success. I tried :
Maybe I missed something... |
For your bug building the doc: looks like a |
Maybe your PR is also the occasion to fix #430, should be just a couple lines 😉. The current default interpolation degrades the output of the image viewer quite a bit by propagating NaNs. |
@@ -35,6 +35,7 @@ dependencies: | |||
- sphinx-design | |||
- sphinx-autodoc-typehints | |||
- sphinxcontrib-programoutput | |||
- sphinx-argparse |
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.
Don't forget, for development-specific dependencies, you have to mirror them manually in "setup.cfg [options.extras_require]" (there's a reminder in the dev-environment
file and another one in a PR template when you open one, but it's been a while here!)
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.
Sorry, I missed that info in the dev-environment.yml
file !
For #430, you probably just need to add this at the start of if "interpolation" not in kwargs.keys():
kwargs.update({"interpolation": "None"}) (the "" around None being very important!! you probably know this annoying matplotlib behaviour... 😅 ) |
That's a good idea and a quick fix. However, on the longer term, I was wondering if we should not have default changes to the rcParams like what is done in Seaborn? |
Finally managed to get the documentation to build properly! The result can bee seen here for the moment: https://geoutils-adehecq.readthedocs.io/en/fix_geoviewer/cli.html The issue was regarding the path to the script |
Perfect! So it works locally + CI with just
Not sure I see exactly, but if you think it is useful it could be an issue for future improvement! |
Added issue #438. |
This PR solves several inconsistencies/bugs in geoviewer.py:
interpolation="None"
the default inRaster.show()
#430Left to do: