ai4er-cdt / geograph

GeoGraph provides a tool for analysing habitat fragmentation and related problems in landscape ecology. GeoGraph builds a geospatially referenced graph from land cover or field survey data and enables graph-based landscape ecology analysis as well as interactive visualizations.
https://geograph.readthedocs.io
MIT License
39 stars 10 forks source link

Feature/unit tests #81

Closed sdat2 closed 2 years ago

sdat2 commented 2 years ago

Hi,

I am adding a some unit tests that will run as a github action for a few different versions of python at each push.

Currently it only pip installs the package and creates the test data, but I will add a few more.

I've added settings in the pytest.ini file that allow code blocks using >>> in python docstrings to act both as unit tests and examples, which should hopefully reduce duplication.

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

sdat2 commented 2 years ago

python-3.10 not able to install rasterio as detailed in #71 .

herbiebradley commented 2 years ago

Feel free to unpin rasterio from the requirements, then run the GitHub actions again! I tested with Python 3.10 and then just pip installed the requirements.txt as normal with the latest version of rasterio and everything seems to work.

sdat2 commented 2 years ago

Hi Herbie, That makes sense, I'll try unpinning the rasterio version.

RE using the requirements.txt file, I guess that that isn't an ideal thing to do if we want to make sure our pip package is compatible? Correct me if I'm wrong, but if you pip install geograph from somewhere else, that wouldn't be used? If that's right, that would undermine people's ability to easily build their packages on top of ours.

sdat2 commented 2 years ago

Ah ok, so everything seems to be ok now for linux/mac, but I should probably fix windows as well for completeness.

herbiebradley commented 2 years ago

Hi Herbie, That makes sense, I'll try unpinning the rasterio version.

RE using the requirements.txt file, I guess that that isn't an ideal thing to do if we want to make sure our pip package is compatible? Correct me if I'm wrong, but if you pip install geograph from somewhere else, that wouldn't be used? If that's right, that would undermine people's ability to easily build their packages on top of ours.

Yes, I just meant that for dev purposes I use the requirements.txt. Of course the actual place to unpin is setup.py, as you have done. :)

herbiebradley commented 2 years ago

Regarding the GDAL Windows issue, you may be able to fix this by including a GDAL wheels download and install in the configuration somehow: https://stackoverflow.com/questions/55583234/installation-fails-for-fiona-and-geopandas-with-gdal-on-python-3-6-on-microsoft

But it's not a big deal for now if it doesn't work - interestingly this was so annoying for geopandas that they don't try to solve it for the user and instead suggest Windows users use pyogrio which is a fiona alternative.

sdat2 commented 2 years ago

Hi, sure, I'm very happy to give up on Windows. I guess we can direct any windows users to this stack exchange answer: https://stackoverflow.com/a/65082853 if they want to use this package.

sdat2 commented 2 years ago

Hi Herbie, I only added 'pipwin' and 'descartes' to the setup.py to try to fix the GDAL error based on some stack exchange answers. I will remove them again in the next commit.

sdat2 commented 2 years ago

(The CI github action was still triggered by master rather than main)

sdat2 commented 2 years ago

I've added only one unit test, which is more of a "smoke test". It might be worth adding some more before we merge this branch in.

sdat2 commented 2 years ago

Ah ok, the CI pylint test is broken, do you think that would be worth fixing?

sdat2 commented 2 years ago

Sure, I can merge and then add some more unit tests in a future pull request.