Skip to content

Bugfix forced log scales in raster plots - #1334

Open
ChrisFairless wants to merge 2 commits into
developfrom
bugfix/custom_raster_plot_transforms
Open

ChrisFairless wants to merge 2 commits into
developfrom
bugfix/custom_raster_plot_transforms

Conversation

@ChrisFairless

Copy link
Copy Markdown
Collaborator

Changes proposed in this PR:

I hope you're ready for another PR from me where the explanation is 20 times longer than the actual fix!

Overview:

When you want to plot a raster for an Exposure or an Impact, so that plot

  • doesn't plot zero values, and
  • applies a non-default function to the raster values before plotting (i.e. not log-scaling),

then CLIMADA unintentionally overrides your choice of function.

More precisely, when Exposures.plot_raster() has fill=False, it overrides the user-provided raster_f parameter.

In detail:

In the method, raster_f is a lambda function that transforms the value of each raster cell and is logarithmic by default (raster_f=lambda x: np.log10((np.fmax(x + 1, 1)))). This bug means you can't actually plot a raster with its raw values (i.e. with raster_f: lambda x: x).

The culprit is in lines 1063-1065:

        if not fill:
            raster = np.where(raster == 0, np.nan, raster)
            raster_f = lambda x: np.log10((np.maximum(x + 1, 1)))

The intention made sense: the default parameters with fill=True use raster_f=lambda x: np.log10((np.fmax(x + 1, 1))). This lambda uses np.fmax, meaning raster_f(np.nan) = 0 meaning that missing values are treated as zeroes and coloured accordingly.

But when fill=False the lines above kick in and override with a new raster_f=lambda x: np.log10((np.maximum(x + 1, 1))). Using np.maximum now means raster_f(np.nan) = np.nan instead, and therefore missing values (and zeroes, due to the mask applied one line earlier) are treated as missing. Which was a neat trick. But it forget that raster_f could be any function, and we don't actually to override it.

The fix:

The solution is just to apply a missing value mask if fill=False instead.

Aside: but wait, what is the method trying to do?

It took me a while to work out the intended behaviour:

  • fill=True: np.nan -> 0
  • fill=False: np.nan -> np.nan
  • fill=True: 0 -> 0
  • fill=False: 0 -> ?
    According to the docstring, this should be 0 BUT the code explicitly masks them, making them np.nan.

I assume the code is exactly what is intended and we always want to mask zero values when fill=False. This makes sense when plotting financially-valued assets.

So that's what I've implemented (fill=False: 0 -> np.nan ) and I've updated the docstring.

Finally: I think what this function actually wanted to accomplish all this time was a mpl.colors.LogNorm colourbar, like in plot.plot_from_gdf. But let's not go into that.

Tests:

Codex wrote an obscure test that took me 10 minutes to understand so I thought it wasn't worth including. A smarter approach would be to move all the masking and transforming into a separate helper function and test that, but I'm not sure we really need it, and this PR is long enough already???

PR Author Checklist

PR Reviewer Checklist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant