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

_shade_map normalizes vmin and vmax instead of recalculating them from normalized zdata #64

Merged

Conversation

maxhollmann
Copy link
Contributor

Hey Raphael, thanks for this library, it's been very useful to me!

I noticed that the vmin & vmax values weren't respected when using shaders. The colorbar used the range set by set_plot_specs, but the colors drawn on the map itself were using the min/max of the normalized values. This fixes it by normalizing the existing vmin/vmax instead of recalculating them after normalization.

@raphaelquast
Copy link
Owner

Hey, @maxhollmann, thanks, nice to hear!
... and of course thanks a lot for taking the time to open a pull request!!

I had a quick look but I could not reproduce the behaviour you're describing...
Could you maybe add a reproducible example so that I can see what's going wrong?

Here's my quick test that does exactly what I would expect it to do:

code to reproduce
from eomaps import MapsGrid
import numpy as np

lon, lat = np.meshgrid(np.linspace(-89, 89, 500), np.linspace(-89, 89, 500))

mg = MapsGrid(1, 2, figsize=(10, 5), wspace=0, left=0.02)
mg.add_feature.preset.coastline()
mg.set_data(np.sqrt(lon**2 + lat**2), lon, lat)
mg.set_shape.shade_raster()
mg.set_plot_specs(vmin=25, vmax=100)

mg[0,0].set_classify_specs("EqualInterval", k=10)

mg.plot_map()
mg.add_colorbar(histbins=50)

for m in mg:
    m.cb.pick.attach.annotate()

image

@raphaelquast raphaelquast added the awaiting followup ... waiting for a response ... label Apr 4, 2022
@maxhollmann
Copy link
Contributor Author

Here's an example that reproduces what I described:

from eomaps import Maps
import numpy as np

lon, lat = np.meshgrid(np.linspace(-89, 89, 500), np.linspace(-89, 89, 500))

data = np.stack([
    np.sqrt(lon**2 + lat**2) + i * 20
    for i in range(4)
])

vmin, vmax = np.min(data), np.max(data)

def prepare_layer(l, i):
    l.add_feature.preset.ocean()
    l.add_feature.preset.coastline()
    l.set_plot_specs(vmin=vmin, vmax=vmax)
    l.set_data(data[i, :, :], lon, lat)
    l.set_shape.shade_raster()
    l.plot_map()
    l.add_colorbar()

m = Maps(crs=4326)
prepare_layer(m, 0)
for i in range(1, data.shape[0]):
    layer = m.new_layer(layer=str(i))
    prepare_layer(layer, i)
    
m.util.layer_slider()

Not sure what makes the difference, I can look into it later today.

@raphaelquast
Copy link
Owner

ooh, i see 😅 yep you are totally right!
The current way for re-evaluating vmin/vmax from the normalized data only works as long as vmin & vmax are inside the data-range. In your case however they are outside the data-range (a perfectly valid use-case) and so using np.nanmin(normalized-data) does not respect the provided limits but always sets them to the lowest data-value.

Thanks for bringing this up!

@raphaelquast
Copy link
Owner

raphaelquast commented Apr 5, 2022

❗ Can you please rebase the branch to derive from the "dev" branch?

  • I'm in the process of releasing v3.4 with a lot of updates and then I could include this right away
  • also pre-commits require an update of the black version to run
    (which is why the tests would fail at the moment... this is already fixed in the dev branch)

@raphaelquast raphaelquast added this to the EOmaps v3.4 milestone Apr 5, 2022
@raphaelquast raphaelquast mentioned this pull request Apr 5, 2022
@raphaelquast raphaelquast added bug Something isn't working and removed awaiting followup ... waiting for a response ... labels Apr 5, 2022
@raphaelquast raphaelquast changed the base branch from master to dev April 5, 2022 09:57
@maxhollmann maxhollmann force-pushed the fix-vmin-max-normalization branch from cb35baf to 323e303 Compare April 5, 2022 10:03
@maxhollmann
Copy link
Contributor Author

Done!

@raphaelquast
Copy link
Owner

wow that was fast! thanks!
sorry... but can you fetch the latest commit I did just right now?
(i realized that I had tests configured to trigger only on "push" and not on arbitrary "pull-requests" which is why they don't seem to run in here (you're the first external contributor so sorry for that 😅 )

@raphaelquast raphaelquast reopened this Apr 5, 2022
@raphaelquast
Copy link
Owner

OK no need to do another rebase... a hotfix in the master-branch (and re-opening the pull-request) did the job equally well!

@codecov-commenter
Copy link

Codecov Report

Merging #64 (323e303) into dev (cbe1248) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##              dev      #64   +/-   ##
=======================================
  Coverage   73.22%   73.22%           
=======================================
  Files          12       12           
  Lines        5323     5323           
=======================================
  Hits         3898     3898           
  Misses       1425     1425           
Impacted Files Coverage Δ
eomaps/eomaps.py 70.97% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbe1248...323e303. Read the comment docs.

@raphaelquast
Copy link
Owner

OK, all checks out fine. Thanks for your contribution!! It'll be in v3.4 (to be released today or tomorrow)

@raphaelquast raphaelquast merged commit 2cc69b2 into raphaelquast:dev Apr 5, 2022
@maxhollmann
Copy link
Contributor Author

Awesome, thanks :)

@maxhollmann maxhollmann deleted the fix-vmin-max-normalization branch April 5, 2022 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants