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

Allow triangulation object in tricountour(f) #13

Merged
merged 12 commits into from
Jan 11, 2025

Conversation

cvanelteren
Copy link
Contributor

Addresses #12

I am awaiting test cases to add to the docs to ensure that this would be fixed and tested for in the future.

@cvanelteren cvanelteren marked this pull request as draft January 7, 2025 15:48
Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Do you know what the @inputs._preprocess_or_redirect("x", "y", "z") decorator does? I suspect there might be issues with the function signature change.

@cvanelteren
Copy link
Contributor Author

Yes it preprocesses some inputs; it can be found in #internals/inputs:257. It redirect some inputs and from what I can tell allows unwrapping of dictionary with a data entry.

@thibaultDrt
Copy link

For information, the arguments for this function in matplotlib are handled here.
It also allow to provide directly a Triangulation object instead of x, y and triangles.

For the decorator, I would know how to handle it here, with different signatures.

@cvanelteren cvanelteren marked this pull request as ready for review January 8, 2025 07:49
@cvanelteren cvanelteren requested a review from beckermr January 10, 2025 10:52
Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

I am pretty sure the changes here break existing functionality that allows people to pass data frames with x/y/z columns to the plotting code. This needs to be checked.

@cvanelteren
Copy link
Contributor Author

No. The input parser that is in place checks whether kwargs contains a data dict which may contain the keys of "x,y,z" which are checked for. I am not sure why this way of parsing is preferred over keeping closer to the mpl way of doing it. The parser checks if some keywords are present. Tbh it seems a bit convoluted to me in general.

@cvanelteren
Copy link
Contributor Author

I have made an adjustment where it passes through both functions now to ensure backwards compatibility. I makes the code a bit overly complicated in my opinion, but perhaps this is the way forward. Happy to know what you think.

@beckermr
Copy link
Collaborator

My intuition is that we don't yet understand the codebase fully and so we should be conservative in terms of how we change things.

Copy link
Collaborator

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

LGTM and nice job on the tests!

@cvanelteren cvanelteren merged commit fb762c5 into Ultraplot:main Jan 11, 2025
2 checks passed
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.

3 participants