-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
There was a problem hiding this 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.
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. |
For information, the arguments for this function in matplotlib are handled here. For the decorator, I would know how to handle it here, with different signatures. |
There was a problem hiding this 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.
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. |
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. |
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. |
There was a problem hiding this 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!
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.