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

Fix several issues in geoviewer #418

Merged
merged 19 commits into from
Jan 18, 2024
Merged

Conversation

adehecq
Copy link
Member

@adehecq adehecq commented Dec 20, 2023

This PR solves several inconsistencies/bugs in geoviewer.py:

Left to do:

@adehecq adehecq requested a review from rhugonnet December 20, 2023 09:05
Copy link
Member

@rhugonnet rhugonnet left a 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.

@adehecq
Copy link
Member Author

adehecq commented Jan 15, 2024

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

So you mean, I should have a subsection in the "Reference" section, e.g. after the API?
Can the documentation of the function be parsed automatically like for the API? If I copy paste the api.rst file and update to list geoviewer.py, would that work?

For test data: We could add a script in examples.py that creates artificial 2D/4D data, or as a pytest fixture.

Right, yes we could have this in the pytest fixture, as it does not make much sense to store the artificial data.

@rhugonnet
Copy link
Member

Yes, exactly! A Command-line interface section in Reference.

To automatically document the function, looks like we can add the sphinx-argparse module: https://sphinx-argparse.readthedocs.io/en/latest/usage.html#basic-usage. The basic usage looks fairly easy. And if we move to click at some point instead of argparse, we can do the same with sphinx-click: https://sphinx-click.readthedocs.io/en/latest/! 🙂

@adehecq
Copy link
Member Author

adehecq commented Jan 16, 2024

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 :

  • adding the docutils.conf file as suggested (2nd option)
  • adding the suggested lines in the doc/source/_static/css/custom.css file (1st option)

Maybe I missed something...

@rhugonnet
Copy link
Member

rhugonnet commented Jan 16, 2024

For your bug building the doc: looks like a distutils error due to Python 3.12 like Andrew's issue: #428.
Adding python >=3.9, <3.12 to the dev-environment and normal environment could solve the issue, and you could close that issue as well!

@rhugonnet
Copy link
Member

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
Copy link
Member

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!)

Copy link
Member Author

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 !

@rhugonnet
Copy link
Member

For #430, you probably just need to add this at the start of Raster.show():

 if "interpolation" not in kwargs.keys():
    kwargs.update({"interpolation": "None"})

(the "" around None being very important!! you probably know this annoying matplotlib behaviour... 😅 )

@adehecq
Copy link
Member Author

adehecq commented Jan 17, 2024

For #430, you probably just need to add this at the start of Raster.show():

 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?

@adehecq adehecq linked an issue Jan 17, 2024 that may be closed by this pull request
@adehecq
Copy link
Member Author

adehecq commented Jan 17, 2024

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 geoviewer.py. If using a relative path, it depends where the sphinx command is ran from. For readthedocs, it seems like "../../bin/geoviewer.py" works, but it fails in test_docs.py because the command is run in the root folder.
Therefore, using simply "geoviewer.py" works better as the script is added to the path by pip install, but is a bit ambiguous. In the future, it would be simpler to use the "module" argument of sphinx-argparse, but require the tool to be located in a module, or use sphinx-click.

@rhugonnet
Copy link
Member

Perfect! So it works locally + CI with just geoviewer? I think that is perfect, this is why we go through the trouble of declaring the module properly through setuptools 🙂

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?

Not sure I see exactly, but if you think it is useful it could be an issue for future improvement!

@adehecq
Copy link
Member Author

adehecq commented Jan 18, 2024

Not sure I see exactly, but if you think it is useful it could be an issue for future improvement!

Added issue #438.

@adehecq adehecq mentioned this pull request Jan 18, 2024
28 tasks
@adehecq adehecq merged commit 27aba1b into GlacioHack:main Jan 18, 2024
10 checks passed
@adehecq adehecq deleted the fix_geoviewer branch January 19, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants