Use geopandas.plot() for Centroids.plot()

CLIMADA-project / climada_python

Python (3.8+) version of CLIMADA

GNU General Public License v3.0

291 stars 115 forks source link

Use geopandas.plot() for Centroids.plot() #896

Open sarah-hlsn opened 2 weeks ago

sarah-hlsn commented 2 weeks ago

Changes proposed in this PR:

use geopandas.plot() to plot centroids
copy & reproject centroids gdf to default CRS before plotting if necessary

This PR fixes SCRUM User Story 86

PR Author Checklist

[x] Read the Contribution Guide
[x] Correct target branch selected (if unsure, select develop)
[x] Descriptive pull request title added
[x] Source branch up-to-date with target branch
[ ] Documentation updated
[ ] Tests updated
[x] Tests passing
[x] No new linter issues
[ ] Changelog updated

PR Reviewer Checklist

[ ] Read the Contribution Guide
[ ] CLIMADA Reviewer Checklist passed
[ ] Tests passing
[ ] No new linter issues

chahank commented 1 week ago

Great work! One more suggestion (see comment). Also, I think the file text.ipynb might be a mistake and should not be commited? Could you also add a small test to the integration tests (no need to test what the plot looks like, just trying to plot with and without crs change once, then we will find out if the plotting fails completely or not).

sarah-hlsn commented 1 week ago

Great work! One more suggestion (see comment). Also, I think the file text.ipynb might be a mistake and should not be commited? Could you also add a small test to the integration tests (no need to test what the plot looks like, just trying to plot with and without crs change once, then we will find out if the plotting fails completely or not).

I think you mean the file test_plot.ipynb? Yes, I was planning on removing it once everything else is working and agreed on. I can definitely add a little test as well, just not sure about how to test without crs change since this is hardcoded in the function? Or do you mean passing centroids in a different crs and checking if the function still works?

chahank commented 1 week ago

I think you mean the file test_plot.ipynb? Yes, I was planning on removing it once everything else is working and agreed on. I can definitely add a little test as well, just not sure about how to test without crs change since this is hardcoded in the function? Or do you mean passing centroids in a different crs and checking if the function still works?

Yes, this file. FYI for the future, it is better to not push this kind of temporary files as they will now stay in the git history.

For the test, I mean just setting the centroids to another crs before plotting.

sarah-hlsn commented 1 day ago

I now added two tests (one to plot centroids with the default crs and one without). They are both running fine, but a bunch of other tests are failing (e.g. related to nightlight and WorldBank data). I am not sure what is causing this, I don't think it is anything I have done? It started happening after I merged the develop branch into this branch, does anyone know of anything that could be causing this? Other than that, I would say I'm done and ready for a final review :)