(De-Duplication) linking validate hatch from hatch module by ShivamPathak99 · Pull Request #28076 · matplotlib/matplotlib

ShivamPathak99 · 2024-04-14T16:42:18Z

PR summary

issue #24797
Applying the discussed changes in rcsetup

PR checklist

"closes [MNT]: de-duplicate hatch validation #24797" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

ShivamPathak99 · 2024-04-15T10:07:40Z

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 · 2024-04-15T15:12:30Z

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 · 2024-04-15T16:53:37Z

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 · 2024-04-18T14:55:45Z

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

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
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
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 · 2024-04-18T20:40:01Z

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

tacaswell · 2024-04-19T14:18:21Z

lib/matplotlib/hatch.py

 def _validate_hatch_pattern(hatch):
    valid_hatch_patterns = set(r'-+|/\xXoO.*')
    if hatch is not None:
+        if not isinstance(hatch, str):


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).

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

matplotlib/lib/matplotlib/path.py

Lines 1025 to 1035 in 199c31f

@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 · 2024-04-21T07:59:57Z

validation in RcParam & HatchModule calls out to rcParam as needed

pro: avoids circular dependency

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

hatchModule will call out rcparams for hatch validation

FIXED: ModuleNotFoundError: No module named 'rcsetup'

ShivamPathak99 · 2024-04-21T19:49:48Z

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

rcomer · 2024-04-21T20:13:02Z

@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. 🤞

lib/matplotlib/hatch.py

ShivamPathak99 · 2024-04-23T18:49:53Z

Why codecov/project/tests failing ?

linking validate hatch from hatch module

522f892

github-actions bot added topic: rcparams topic: hatch labels Apr 14, 2024

ShivamPathak99 marked this pull request as draft April 15, 2024 10:03

tacaswell reviewed Apr 19, 2024

View reviewed changes

shifted hatch validation entirely to rcsetup

c57111a

hatchModule will call out rcparams for hatch validation

github-actions bot removed the topic: rcparams label Apr 21, 2024

ShivamPathak99 added 3 commits April 21, 2024 16:39

Merge branch 'matplotlib:main' into hatch_validation

0c542d6

fixed rcsetup import error

4e8cd24

FIXED: ModuleNotFoundError: No module named 'rcsetup'

Countering None Inputs

23b55a2

github-actions bot added the topic: rcparams label Apr 21, 2024

added X hatch in validaion

8b109bd

ShivamPathak99 marked this pull request as ready for review April 22, 2024 02:46

tacaswell reviewed Apr 22, 2024

View reviewed changes

lib/matplotlib/hatch.py Show resolved Hide resolved

ShivamPathak99 added 3 commits April 22, 2024 21:29

Create 28076-SP.rst

50e5910

Update 28076-SP.rst

50ff42d

Update 28076-SP.rst

9f7d527

github-actions bot added the status: needs rebase label May 8, 2024

	@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)

Uh oh!

Conversation

ShivamPathak99 commented Apr 14, 2024

PR summary

PR checklist

Uh oh!

ShivamPathak99 commented Apr 15, 2024

Uh oh!

tacaswell commented Apr 15, 2024

Uh oh!

story645 commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

story645 commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

story645 commented Apr 18, 2024

Uh oh!

tacaswell Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

ShivamPathak99 Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

tacaswell Apr 22, 2024

Choose a reason for hiding this comment

Uh oh!

ShivamPathak99 commented Apr 21, 2024

Uh oh!

ShivamPathak99 commented Apr 21, 2024

Uh oh!

rcomer commented Apr 21, 2024

Uh oh!

Uh oh!

ShivamPathak99 commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

story645 commented Apr 15, 2024 •

edited

Loading

story645 commented Apr 18, 2024 •

edited

Loading