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

[sarah-hlsn]

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

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

PR Reviewer Checklist

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

[ValentinGebhart]

Do I understand this correctly that now you cannot plot the centroids on an existing axis anymore? Was this a functionality that is not not needed or that would be difficult to implement using gdf.plot()?

[chahank]

That would be I think a suboptimal idea. Both the axis and the figures should be possible arguments for the method. Otherwise it is very difficult to change projections, figure sizes, plot several centroids in a multi-axis etc.

[sarah-hlsn]

Hi both, I see your point and I briefly talke to Sam & Lukas about this as well. We thought since the main use of this function would be to get a quick overview of the centroids, this very simple version enables users to do that. Now that the centroids class is a gdf, any more fancy type of plotting can easily be done using the geopandas plotting function.
Of course it would be nice to be able to plot on an existing axis, but as far as I am aware, in order to create a cartopy axis instance, you have to pass the projection argument when creating the axis, and it is not possible to add it retroactively to an existing axis. So I am not sure how to make this "fool-proof" for axis objects created by the user independently outside the function. Regarding projections, we could make that an argument if you think it would add a lot. Let me know what you think :)

[chahank]

Good points! However, I see no reason not to allow a user to pass their cartopy axis or figure if they want to.

[sarah-hlsn]

Yes, we can definitely allow it, but how would you deal with the case that the user did not create a cartopy axis? write a check in the function and throw a warning? Or something else?

[chahank]

I am not sure that there is a need to check if it is written in the docstring.

[sarah-hlsn]

Mhm I think if we're writing the function flexibly in the spirit of user-friendliness shouldn't we also make any potential error clear without the person having to go look in the docstrings? But maybe the error will be clear anyways... I will adjust the method and see what happens

[chahank]

I agree. At the same time, we do not want to start checking for errors in using other packages (here geopandas).

Let's see what your test says :D

[sarah-hlsn]

The default error you would get when passing the wrong type of axis does not make it obvious what the issue is, so I added a custom error message to the function. The user can now plot the centroids on a custom axis. I also ensured that plotting with different projections on user-defined axes is possible. Ready for a new round of reviews :)

[chahank]

This is a bit inconsistent now. Here, you set the projection always to PlateCarree. Below, you use the method to_default_crs. It is then neither clear nor guaranteed that these are the same. Why not have the ccrs as a method variable, and then use centroids.to_crs instead?

[sarah-hlsn]

Well PlateCarree and DEF_CRS always work together, which is why I am always checking if Centroids.crs == DEF_CRS (and reprojecting if not) and giving PlateCarree to the transform argument (line 517).

The line you're commenting on here is just defining that the projection the centroids are plotted on is PlateCarree. So this is then a question, whether we want to add projection as an argument to the function for the user to define. Right now the user can do that if they pass a previously defined axis to the function.

I am just not sure how it would work having both axis and projection arguments, since it would result in an error about passing multiple projection arguments... Which we could handle for sure but then this is more lines of code and I thought we wanted to keep this function simple

[chahank]

good points! Maybe then we should keep it as is.

[sarah-hlsn]

Ok great, I'll keep it as is unless someone can think of an elegant way to do this that I haven't considered

[chahank]

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]

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]

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]

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

[ValentinGebhart]

To me the PR looks fine. Maybe two small comments:

• There are some functions with keyword arguments before variable positional arguments. Should we correct the order, or do we even need the *args input here?
  def plot(self, axis=None, figsize=(9, 13), *args, **kwargs)
centroids_plot.gdf.plot(ax=axis, transform=ccrs.PlateCarree(), *args, **kwargs)
self.gdf.plot(ax=axis, transform=ccrs.PlateCarree(), *args, **kwargs)

• The test functions only test whether the plot works without throwing an error, they don't test if the function plots 'correctly'. I don't know how one could improve this, just want to mention this.

[emanuel-schmid]

The test functions only test whether the plot works without throwing an error, they don't test if the function plots 'correctly'.

Meaningfully testing a plot within a unit test is pretty hard! You could do image analysis which is way out of scope or you could inspect the plot's data, which is tedious and difficult to do it right. Therefore, for climada, we only check whether we can plot at all, so far.

[emanuel-schmid]

There are some functions with keyword arguments before variable positional arguments. Should we correct the order, or do we even need the *args input here?

I guess it is meant to be a keyword-only function? If that's the case the signature should be:

def plot(self, *, axis=None, figsize=(9, 13), **kwargs):

[sarah-hlsn]

Hi Valentin and Emanuel,

Thanks for bringing up / adding to the *args question, I myself wasn't sure how to do this correctly, whether to include *args at all. I will make the adjustment as suggested by Emanuel, I just have a question: what does the single * do here?
def plot(self, *, axis=None, figsize=(9, 13), **kwargs):

[emanuel-schmid]

@sarah-hlsn it limits the number of non-keyword-arguments.
With this signature one must call it with named arguments: centroids.plot(figsize=figsize) When you try to run it with centroids.plot(figsize) it will fail.

[peanutfun]

Failing Petals compatibility tests for Python 3.10+ are due to CLIMADA-project/climada_petals#132 and can be ignored