Skip to content

fix late binding and proper reversal for funcs #296

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

Merged
merged 6 commits into from
Jul 2, 2025

Conversation

cvanelteren
Copy link
Contributor

A secondary issue came up in #294 where a callable is not properly inverse.

This PR fixes a bug in the reversal of colormap segment data where callable segments were incorrectly captured due to Python’s late binding behavior in closures. Specifically, lambdas inside the dictionary comprehension referenced the loop variable data, which caused all callables to use the last data value. This is resolved by binding data as a default argument in the lambda. Explicit segment lists are unaffected and still reversed correctly.

@cvanelteren cvanelteren force-pushed the fix-binding-bug-reversal-cmap branch from f0342a3 to 6f56ae5 Compare June 29, 2025 13:10
@cvanelteren cvanelteren force-pushed the fix-binding-bug-reversal-cmap branch from 6f56ae5 to f23710b Compare June 29, 2025 13:11
@cvanelteren
Copy link
Contributor Author

cvanelteren commented Jun 29, 2025

image

import ultraplot as uplt

fig, ax = uplt.subplots()

cmaps = ["jet", "jet_r", "rainbow", "rainbow_r"]
for i, loc in enumerate(["r", "l", "t", "b"]):
    ax.colorbar(uplt.Colormap(cmaps[i]), loc=loc, label=cmaps[i])
uplt.show(block=1)

@cvanelteren cvanelteren added this to the v1.57.2 milestone Jun 29, 2025
@cvanelteren cvanelteren added the bug Something isn't working label Jun 29, 2025
@cvanelteren cvanelteren requested a review from beckermr June 29, 2025 13:18
Copy link

codecov bot commented Jun 29, 2025

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ultraplot/colors.py 75.00% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@@ -3156,7 +3157,7 @@ def _translate_key(self, original_key, mirror=True):

# Try mirroring the non-lowered key
if reverse:
original_key = original_key.strip("_r")
original_key = original_key.rstrip("_r")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method removes any of the characters in _r, not _r if the string ends with _r. I think you may want something like original_key.rsplit("_r", maxsplit=1)[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, but then we should replace all strip functions by a split you suggest. My edit here merely replaces it with a more appropriate function. I think the original strip is wrong as it also deletes leading "_r".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right we should do that fix. We'll need another PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also include it in here, although I think this issue will not be a major things as the _r is added by ultraplot. If the colormap ends with _r it will be listed as {cmapnameendingwith_r}_r which should then split fine I think.

@cvanelteren
Copy link
Contributor Author

We can go ahead and merge this I guess @beckermr?

@beckermr
Copy link
Collaborator

beckermr commented Jul 2, 2025

Didn't you want to fix the string _r thing here too?

@cvanelteren
Copy link
Contributor Author

Ah ok that wasn't entirely clear. Will do that now

@cvanelteren
Copy link
Contributor Author

done ;-)!

@beckermr beckermr merged commit 9a1f616 into Ultraplot:main Jul 2, 2025
25 checks passed
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.

2 participants