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

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

ShivamPathak99
Copy link
Contributor

PR summary

issue #24797
Applying the discussed changes in rcsetup

PR checklist

@ShivamPathak99
Copy link
Contributor Author

I think py:obj reference target not found: rcsetup.validate_hatch is causing test fails.
as I have changed validate_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 commented Apr 15, 2024

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 commented Apr 18, 2024

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.

@@ -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 on Patch.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
Contributor Author

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 an lrucache 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)
def hatch(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.
"""
from matplotlib.hatch import get_path
return (get_path(hatchpattern, density)
if hatchpattern is not None else None)

Is part of the code path between Patch.set_hatch and rendering a hatched patch. The lru_cache means we will avoid re-computing the hatch path. The lru_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
Contributor Author

validation in RcParam & HatchModule calls out to rcParam as needed

  • pro: avoids circular dependency

Sorry for delay, figuring out option 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
Contributor Author

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

@rcomer
Copy link
Member

rcomer commented Apr 21, 2024

@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 ShivamPathak99 marked this pull request as ready for review April 22, 2024 02:46
@ShivamPathak99
Copy link
Contributor Author

Why codecov/project/tests failing ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MNT]: de-duplicate hatch validation
4 participants