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.

Sign up for GitHub

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

Jump to bottom

[MNT]: de-duplicate hatch validation #24797

Open
story645 opened this issue Dec 21, 2022 · 5 comments · May be fixed by #27108 or #28076
Open

[MNT]: de-duplicate hatch validation #24797

story645 opened this issue Dec 21, 2022 · 5 comments · May be fixed by #27108 or #28076
Labels
Maintenance topic: hatch

Comments

@story645
Copy link
Member

Summary

In #17926 code to validate hatches got added to the hatch module:

matplotlib/lib/matplotlib/hatch.py

Lines 182 to 196 in 5100378

def _validate_hatch_pattern(hatch):
valid_hatch_patterns = set(r'-+|/\xXoO.*')
if hatch is not None:
invalids = set(hatch).difference(valid_hatch_patterns)
if invalids:
valid = ''.join(sorted(valid_hatch_patterns))
invalids = ''.join(sorted(invalids))
_api.warn_deprecated(
'3.4',
removal='3.8', # one release after custom hatches (#20690)
message=f'hatch must consist of a string of "{valid}" or '
'None, but found the following invalid values '
f'"{invalids}". Passing invalid values is deprecated '
'since %(since)s and will become an error %(removal)s.'
)

and there's near identical code in rc validation that went in in #4686:

matplotlib/lib/matplotlib/rcsetup.py

Lines 580 to 592 in 5093150

def validate_hatch(s):
r"""
Validate a hatch pattern.
A hatch pattern string can have any sequence of the following
characters: ``\ / | - + * . x o O``.
"""
if not isinstance(s, str):
raise ValueError("Hatch pattern must be a string")
_api.check_isinstance(str, hatch_pattern=s)
unknown = set(s) - {'\\', '/', '|', '-', '+', '*', '.', 'x', 'o', 'O'}
if unknown:
raise ValueError("Unknown hatch symbol(s): %s" % list(unknown))
return s

They diverge slightly in that the hatch version has a deprecation warning but I think it should also apply to the rc version?

Proposed fix

Make the validation function in hatch.py the canonical validator and either deprecate the rcsetup validation function or turn it into a thin shim over the one in hatch.
@oscargus
Copy link
Contributor

oscargus commented Dec 21, 2022 •
edited

Also relevant: #20690 and #21108 (as in the "proper" fix is probably to introduce a HatchStyle and then have a single validator).

@anntzer
Copy link
Contributor

anntzer commented Jan 1, 2023

Assuming we want to allow custom hatches at some point (which seems to be the general agreement in #20690, and I tend to agree as well), the practical solution may just be to get rid of validate_hatch and allow any string in rcParams (possibly later exploding at runtime), just like we allow any string for image.cmap (and not just known colormap names) as the corresponding colormap may well not be registered yet (e.g. if reading from matplotlibrc and registering the colormap at runtime).

@story645
Copy link
Member Author

story645 commented Jan 1, 2023

just like we allow any string for image.cmap

why not do something like the marker API, where either pass a string that's currently valid or a 'HatchStyle' object?

@anntzer
Copy link
Contributor

anntzer commented Jan 2, 2023 •
edited

That won't work if you want to specify in your matplotlibrc a hatchstyle that'll be defined by a third-party library.

@EricRLeao1311 EricRLeao1311 linked a pull request Oct 16, 2023 that will close this issue
Open
1 task
@ShivamPathak99 ShivamPathak99 linked a pull request Apr 14, 2024 that will close this issue
Open
5 tasks
@story645
Copy link
Member Author

story645 commented Apr 14, 2024 •
edited

That won't work if you want to specify in your matplotlibrc a hatchstyle that'll be defined by a third-party library.

I don't know if anyone's opened an issue for this, but on one of the calls we've discussed the idea of having a registry system for hatches, kinda like how we do for colormaps? Which I think ties into your suggestion on how we do validation like we do for cmap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Maintenance topic: hatch
Projects
None yet
Milestone
No milestone
3 participants
@story645 @anntzer @oscargus