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

Migrate RAVEN GIS utilities and fix the documentation #68

Merged
merged 48 commits into from
Mar 9, 2021

Conversation

Zeitsperre
Copy link
Member

@Zeitsperre Zeitsperre commented Mar 2, 2021

This PR migrates all but one function (only used in RAVEN WPS) from raven.utils.py. This PR also addresses most of the FIXME notes pointing to unexpected/broken behaviour. Things should now be much more resilient/functional than previously.

This PR also refactors ravenpy/utils.py into utilities (analysis, geo, checks, io). This refactoring was done synonymously with Ouranosinc/raven#335, so changes are already up-to-date on that PR. Some utilities have been migrated there (parse_lonlat()).

As a final added benefit, the hydro_routing functions for collecting and performing upstream sub-basin identification have now been written in this PR. Tests for these functions are also written and passing.

This PR now fixes #44

EDIT: With the merge from #70 this PR is also set to close #58

@Zeitsperre Zeitsperre added the enhancement New feature or request label Mar 2, 2021
@Zeitsperre Zeitsperre requested review from huard and cjauvin March 2, 2021 22:25
@Zeitsperre Zeitsperre self-assigned this Mar 2, 2021
@cjauvin
Copy link
Collaborator

cjauvin commented Mar 2, 2021

I'm sorry I'm a little bit confused: we are migrating what from what exactly? I don't understand the migration aspect just by looking at the code (admittedly I looked superficially so far). Also, since it seems this is related to it, are we addressing #44? If not, should we? I really find that confusing to have both utils.py and utilities.

@Zeitsperre
Copy link
Member Author

I am migrating all the GIS-like file operations that RAVEN uses into RavenPy for easier maintenance. This PR is meant to synchronize the two (so that we don't have redundancies in both projects). If we want to move/split/rename utils.py, this would be the PR to do it.

@Zeitsperre
Copy link
Member Author

Zeitsperre commented Mar 2, 2021

I've already moved all non-owslib functions out of utilities/gis.py and renamed it utilities/geoserver.py. We can divvy the functions in utils.py as needed. Do you want me to propose a schema?

@cjauvin
Copy link
Collaborator

cjauvin commented Mar 2, 2021

Do you want me to propose a schema?

If you don't mind that would be great!

tests/test_geoserver.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@huard huard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAILED tests/test_geoserver.py::TestWCS::test_get_raster_wcs - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DATA environment vari...
FAILED tests/test_utils.py::TestGdalOgrFunctions::test_dem_properties - AssertionError: 
FAILED tests/test_utils.py::TestGenericGeoOperations::test_raster_warp - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DATA enviro...
FAILED tests/test_utils.py::TestGenericGeoOperations::test_warped_raster_slope - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DAT...
FAILED tests/test_utils.py::TestGenericGeoOperations::test_warped_raster_aspect - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DA...

@Zeitsperre
Copy link
Member Author

FAILED tests/test_geoserver.py::TestWCS::test_get_raster_wcs - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DATA environment vari...
FAILED tests/test_utils.py::TestGdalOgrFunctions::test_dem_properties - AssertionError: 
FAILED tests/test_utils.py::TestGenericGeoOperations::test_raster_warp - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DATA enviro...
FAILED tests/test_utils.py::TestGenericGeoOperations::test_warped_raster_slope - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DAT...
FAILED tests/test_utils.py::TestGenericGeoOperations::test_warped_raster_aspect - rasterio.errors.CRSError: The EPSG code is unknown. Unable to open EPSG support file gcs.csv.  Try setting the GDAL_DA...

Hmmm... Is this in a fresh environment? What version of GDAL is in your system libraries?

@cjauvin
Copy link
Collaborator

cjauvin commented Mar 3, 2021

Yes I know this error, you need to do: unset GDAL_DATA in your env. It's a Fiona thing I'm pretty sure, no idea why it does that.

@Zeitsperre
Copy link
Member Author

Just did a rebuild with Anaconda and another with venv. The python-GDAL version you use should match the GDAL C-libraries (system or conda-forge).

@Zeitsperre
Copy link
Member Author

Zeitsperre commented Mar 4, 2021

@cjauvin I think I have a schema for refactoring the utils.py:

io.py
--> address_append
--> archive_sniffer
--> generic_extract_archive
--> get_bbox
--> crs_sniffer
--> raster_datatype_sniffer

analysis.py
--> circular_mean_aspect
--> gdal_aspect_analysis
--> gdal_slope_analysis
--> dem_prop
--> geom_prop

checks.py
--> multipolygon_check
--> feature_contains
--> boundary_check
--> single_file_check

geo.py
--> generic_raster_clip
--> generic_raster_warp
--> generic_vector_reproject
--> geom_transform

MOVE --> raven.utils.parse_lonlat (only used in WPS processes.

If this works for you, I can move forward with this. It should be one of the last thing I do here before merge.

@cjauvin
Copy link
Collaborator

cjauvin commented Mar 4, 2021

@Zeitsperre Thanks a lot I really appreciate it!

…ted imports for ravenpy tests, and raven-wps processes, and parse_lonlat refactored to raven
@Zeitsperre
Copy link
Member Author

@cjauvin @huard This is good to be reviewed!

ravenpy/utilities/geo.py Outdated Show resolved Hide resolved
ravenpy/utilities/geo.py Outdated Show resolved Hide resolved
ravenpy/utilities/geo.py Outdated Show resolved Hide resolved
ravenpy/utilities/geo.py Outdated Show resolved Hide resolved
ravenpy/utilities/geo.py Outdated Show resolved Hide resolved
tests/test_emulators.py Outdated Show resolved Hide resolved
tests/test_geoserver.py Outdated Show resolved Hide resolved
@cjauvin
Copy link
Collaborator

cjauvin commented Mar 6, 2021

According to this: readthedocs/readthedocs.org#1139 RTD is not able to run sphinx-apidoc on its own, which explains our troubles with modules.rst and all the generated rst files. It seems we have two choices:

(1) Commit all its generated files
(2) Use a hack to have RTD run sphinx-apidoc (via conf.py), as a few comments suggest.

@Zeitsperre
Copy link
Member Author

Zeitsperre commented Mar 8, 2021

I find this "hack" to be somewhat reasonable:

def run_apidoc(_):
	from sphinx.apidoc import main
	import os
	import sys
	sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
	cur_dir = os.path.abspath(os.path.dirname(__file__))
	module = os.path.join(cur_dir,"..","ravenpy")
	main(['-e', '-o', cur_dir, module, '--force'])

def setup(app):
	app.connect('builder-inited', run_apidoc)

Shall we implement it?

@Zeitsperre
Copy link
Member Author

Zeitsperre commented Mar 8, 2021

Leaving this here since this needs to be addressed: Toblerity/Fiona#986

Edit: For clarification, this warning is a regression in Fiona 1.8.18. Feel free to ignore any raised warning about sequential read.

@Zeitsperre
Copy link
Member Author

A final comment: I'm updating the installation docs to add another step. So long as there are no additional python libraries to install, running pip install --editable ".[dev]" --install-option="--with-binaries" will work (downloads and installs the RavenC and Ostrich binaries). This process is pretty terrible for the moment, but this is the only working method at time of writing. I will merge after this change is made.

Thanks for the help @cjauvin and @huard !

@Zeitsperre Zeitsperre merged commit 990ef0f into master Mar 9, 2021
@Zeitsperre Zeitsperre deleted the raven_gis_ports branch March 9, 2021 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTD docs is broken Refactor ravenpy.utils somewhere into ravenpy.utilities?
3 participants