Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Replace hardcoded parameter names when creating error bars#23376

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

wqian94
Copy link
Contributor

@wqian94wqian94 commentedJun 30, 2022
edited
Loading

PR Summary

Useinspect.signature to automagically check whether akwarg is a
line argument, rather than relying on a hardcoded list. See issue#23375

PR Checklist

Tests and Styling

  • [❌] Has pytest style unit tests (andpytest passes).
  • [✔️] IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [✔️] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

Use `inspect.signature` to automagically check whether a `kwarg` is aline argument, rather than relying on a hardcoded list.
Copy link

@github-actionsgithub-actionsbot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping@matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join uson gitter for real-time discussion.

For details on testing, writing docs, and our review process, please seethe developer guide

We strive to be a welcoming and open project. Please follow ourCode of Conduct.

@wqian94wqian94 marked this pull request as draftJune 30, 2022 21:44
@oscargus
Copy link
Member

I think that although the idea of the PR is good, it is probably quite hard to get through as there is quite a bit of magic going on with the keyword handling, including aliases. However, it is no way intended to discourage you of doing this, it may be possible for some classes, but be prepared for that it can be quite a bit of hassle, closing in to impossible in some cases.

@timhoffm
Copy link
Member

timhoffm commentedJul 4, 2022
edited
Loading

@wqian94 thanks for the PR!

I'm hesitant whether the automatism of introspection is useful here. A subset of the parameters should be passed on. While I think this proposal would currently work correctly, it's not a-priori correct that we should not pass any Line2D parameters to the caps. e.g.

  • color should indeed be passed on. The introspection filters it out, and it's only still working because we have special-handling code for that below, which re-introduces the parameter.
  • Other properties likealpha should propagate as well. This works here only becausealpha is handled in the Line2D signature via**kwargs. If we would make that explicit inLine2D.__init__, propagation would become broken.

Therefore, it may well be that future code changes will break the correct working of the introspection mechanism. The effect is a change in the visual output (i.e. some properties are not propagated), but no error, which is hard to detect.
OTOH, hard-coding some non-supported parameters is defensive - only remove what you are sure is not working. This may not be complete, but if we pass too much, users will get an error like in#23375, which we can fix explicitly.

Since all parameters will be passed to `LineCollection.set()`, we canjust use that as the primary filter. If there are other keys that shouldbe excluded, they should be handled separately, such as by overwritingthe value, as is done for some keys.Notably, `set()` is a kwarg-based key-value updater that uses the`LineCollection`'s `kwargs` specification to perform the appropriateupdates. This change allows for all of the gatekeeping to becentralized.
@wqian94
Copy link
ContributorAuthor

Sorry, been away for the weekend.

I'm working on some changes, but it sounds like there isn't much appetite for this, so maybe I'll just keep it as part of a local build instead.

@timhoffm
Copy link
Member

@wqian94 Yes, I think just addingmarkerfacecoloralt to the hardcoded list. Is the better approach here. Would you be willing to make a PR for that?

@wqian94
Copy link
ContributorAuthor

Sure, I'll submit a separate PR.

@wqian94
Copy link
ContributorAuthor

Replaced with PR#23475

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

@github-actionsgithub-actions[bot]github-actions[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@wqian94@oscargus@timhoffm

[8]ページ先頭

©2009-2025 Movatter.jp