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

(De-Duplication) linking validate hatch from hatch module#28076

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

Open
ShivamPathak99 wants to merge9 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromShivamPathak99:hatch_validation

Conversation

ShivamPathak99
Copy link
Contributor

PR summary

issue#24797
Applying the discussed changes inrcsetup

PR checklist

@ShivamPathak99
Copy link
ContributorAuthor

I thinkpy:obj reference target not found: rcsetup.validate_hatch is causing test fails.
as I have changedvalidate_hatch into a alias name.

validate_hatch=_validate_hatch_pattern

Any fixes for this error?

@tacaswell
Copy link
Member

rcparams is imported very early in import process so this change adds a circular import (matplotlib/init.py -> rcparams.py -> hatch.py -> matplotlib/init.py).

If we want to do this merging, then the cannonical location needs to be in rcparams (as funny as that seems).

@story645
Copy link
Member

story645 commentedApr 15, 2024•
edited
Loading

If we want to do this merging, then the cannonical location needs to be in rcparams (as funny as that seems).

What about going the other direction and doing the validation at run time, which I think is kind of what@anntzer is suggesting in#24797 (comment) b/c it allows for setting third-party hatches in rcParams.

@story645
Copy link
Member

story645 commentedApr 18, 2024•
edited
Loading

Ok, so I'm just gonna summarize my understanding of three options:

  1. arbitrary string + runtime validation in HatchModule[MNT]: de-duplicate hatch validation #24797 (comment))
    • pro: makes it easier to allow third party hatches
    • con: validation is done in runtime - but may be able to call out inside the RcParams init for this the way we do other runtime Rc Validation
  2. validation in RcParam & HatchModule calls out to rcParam as needed
    • pro: avoids circular dependency
    • con: separation of concerns- valid hatches are effectively defined in RcParam rather than in hatch, so have to call out to rcParam for runtime validation. May end up needing two chains of fallback logic for external Hatchs -> inside RcParams and inside Hatches
  3. leave duplicated validation functions:
    • pro: no changes needed
    • con: two sources of truth for valid hatch specs so increased potential for drift/inconsistency and maintenance burden of keeping (remembering to keep) the two specs in sync

@story645
Copy link
Member

@ShivamPathak99 we discussed this on the call and decision was either do 2. or 3.

ShivamPathak99 reacted with thumbs up emoji

@@ -182,6 +182,8 @@ def __init__(self, hatch, density):
def _validate_hatch_pattern(hatch):
valid_hatch_patterns = set(r'-+|/\xXoO.*')
if hatch is not None:
if not isinstance(hatch, str):
Copy link
Member

Choose a reason for hiding this comment

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

This is an subtle API change that needs to be document as currently

b.set_hatch(('x','x'))

works. The docstring onPatch.set_hatch is not clear about the type on the input should be.

We do run the hatches through an lrucache so only hashable things work (and it looks like we used as a key in a dictionary before that).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Working on it,
I'm a bit lost on -

We do run the hatches through anlrucache so only hashable things work (and it looks like we used as a key in a dictionary before that).

Could you break it down for me

Copy link
Member

Choose a reason for hiding this comment

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

@staticmethod
@lru_cache(8)
defhatch(hatchpattern,density=6):
"""
Given a hatch specifier, *hatchpattern*, generates a `Path` that
can be used in a repeated hatching pattern. *density* is the
number of lines per unit square.
"""
frommatplotlib.hatchimportget_path
return (get_path(hatchpattern,density)
ifhatchpatternisnotNoneelseNone)

Is part of the code path betweenPatch.set_hatch and rendering a hatched patch. Thelru_cache means we will avoid re-computing the hatch path. Thelru_cache is backed by a dictionary (eventually) with the function parameters as the key so only inputs that are hashable can be used.

@ShivamPathak99
Copy link
ContributorAuthor

validation in RcParam & HatchModule calls out to rcParam as needed

  • pro: avoids circular dependency

Sorry for delay, figuring outoption 2.

  1. validation in RcParam & HatchModule calls out to rcParam as needed

    • pro: avoids circular dependency
    • con: separation of concerns- valid hatches are effectively defined in RcParam rather than in hatch, so have to call out to rcParam for runtime validation. May end up needing two chains of fallback logic for external Hatchs -> inside RcParams and inside Hatches

hatchModule will call out rcparams for hatch validation
@ShivamPathak99
Copy link
ContributorAuthor

Have I done it right, some tests are still failing.
Any suggestions ?''🤔

@rcomer
Copy link
Member

@ShivamPathak99 some of the tests are a bit flakey. I’ve re-run the failed one so let’s see how it goes this time. 🤞

ShivamPathak99 reacted with thumbs up emoji

@ShivamPathak99ShivamPathak99 marked this pull request as ready for reviewApril 22, 2024 02:46
@ShivamPathak99
Copy link
ContributorAuthor

Whycodecov/project/tests failing ?

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

@tacaswelltacaswelltacaswell left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[MNT]: de-duplicate hatch validation
4 participants
@ShivamPathak99@tacaswell@story645@rcomer
[8]ページ先頭
©2009-2025 Movatter.jp