Deduplicate hatch validate #27108

EricRLeao1311 · 2023-10-16T23:11:54Z

PR summary

Hello! I replaced the deprecated warning from _validate_hatch_pattern() function in hatch.py with the validate_hatch() warn from rcsetup.py. I also make validate_hatch() warn from rcsetup.py use the _validate_hatch_pattern() function. This removed the deprecated warning from the hatch validation code and made it the canonical validator for all hatch patterns.

PR checklist

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

I replaced the deprecated warning from _validate_hatch_pattern() function in hatch.py with the validate_hatch() warn from rcsetup.py. I also make validate_hatch() warn from rcsetup.py use the _validate_hatch_pattern() function. This removed the deprecated warning from the hatch validation code and made it the canonical validator for all hatch patterns.

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

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

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

ksunden

Please revert all of the unrelated changes, especially those that provide desired functionality such as warnings or additional validations for valid types.

ksunden · 2023-10-16T23:16:54Z

lib/matplotlib/hatch.py

+            message = f"""Unknown hatch symbol(s): {invalids}.
+                          Hatch must consist of a string of {valid}"""
+            raise ValueError(message)
+    else:
+        raise ValueError("Hatch pattern must be a string")


The referenced issue RE: custom hatches (#20690) is still open...

That said, it has been warning for a while now, and there is no real motion on that issue, I guess we may be able to expire this deprecation even without that.

Also the original wording included the valid value of None, which this wording omits.

Thoughts @timhoffm?

I'm sorry, I had already started working on the project before and mistakenly picked an older version of the code to make changes (from last year). I will rectify this and restore all the functions that I accidentally removed. I will resubmit my pull request. I'm new to the project and hope to be able to contribute.

You can update your pull request by pushing additional commits (or force-pushing a rebased branch) to the same branch, please do not open new pull requests, this helps keep the conversation in one place so we can more easily see that things are addressed.

ksunden · 2023-10-16T23:18:05Z

lib/matplotlib/rcsetup.py

@@ -179,7 +180,7 @@ def _make_type_validator(cls, *, allow_none=False):

    def validator(s):
        if (allow_none and
-                (s is None or cbook._str_lower_equal(s, "none"))):
+                (s is None or isinstance(s, str) and s.lower() == "none")):


why not use the cbook method here, this seems unrelated to the stated change

ksunden · 2023-10-16T23:18:25Z

lib/matplotlib/rcsetup.py

-def _validate_papersize(s):
-    # Re-inline this validator when the 'auto' deprecation expires.
-    s = ValidateInStrings("ps.papersize",
-                          ["figure", "auto", "letter", "legal", "ledger",
-                           *[f"{ab}{i}" for ab in "ab" for i in range(11)]],
-                          ignorecase=True)(s)
-    if s == "auto":
-        _api.warn_deprecated("3.8", name="ps.papersize='auto'",
-                             addendum="Pass an explicit paper type, figure, or omit "
-                             "the *ps.papersize* rcParam entirely.")
-    return s
-
-


This removal seems unrelated to the hatch changes. And the replacement does not provide the warning that we do want to have there.

ksunden · 2023-10-16T23:23:39Z

lib/matplotlib/rcsetup.py

+def validate_hatch(hatch):
+    _validate_hatch_pattern(hatch)


If we do just import this, I don't see much reason to define a method that just calls it...

I guess these are "public", (and documented), so that could be a reason, but I would spell this as

validate_hatch = _validate_hatch_pattern

ksunden · 2023-10-16T23:24:08Z

lib/matplotlib/rcsetup.py

@@ -615,7 +592,7 @@ def _validate_minor_tick_ndivs(n):
    two major ticks.
    """

-    if cbook._str_lower_equal(n, 'auto'):
+    if isinstance(n, str) and n.lower() == 'auto':


again, cbook method is fine and this is an unrelated change

ksunden · 2023-10-16T23:26:44Z

lib/matplotlib/rcsetup.py

-# A validator dedicated to the named legend loc
-_validate_named_legend_loc = ValidateInStrings(
-    'legend.loc',
-    [
-        "best",
-        "upper right", "upper left", "lower left", "lower right", "right",
-        "center left", "center right", "lower center", "upper center",
-        "center"],
-    ignorecase=True)
-
-
-def _validate_legend_loc(loc):
-    """
-    Confirm that loc is a type which rc.Params["legend.loc"] supports.
-
-    .. versionadded:: 3.8
-
-    Parameters
-    ----------
-    loc : str | int | (float, float) | str((float, float))
-        The location of the legend.
-
-    Returns
-    -------
-    loc : str | int | (float, float) or raise ValueError exception
-        The location of the legend.
-    """
-    if isinstance(loc, str):
-        try:
-            return _validate_named_legend_loc(loc)
-        except ValueError:
-            pass
-        try:
-            loc = ast.literal_eval(loc)
-        except (SyntaxError, ValueError):
-            pass
-    if isinstance(loc, int):
-        if 0 <= loc <= 10:
-            return loc
-    if isinstance(loc, tuple):
-        if len(loc) == 2 and all(isinstance(e, Real) for e in loc):
-            return loc
-    raise ValueError(f"{loc} is not a valid legend location.")
-
-


Why were these removed?

legend_loc also accepts non stringed entries, such as (0.8, 0.8) So the replacement which only accepts the strings is incorrect.

ksunden · 2023-10-16T23:27:06Z

lib/matplotlib/rcsetup.py

@@ -1229,7 +1162,6 @@ def _convert_validator_spec(key, conv):
    "figure.autolayout":       validate_bool,
    "figure.max_open_warning": validate_int,
    "figure.raise_window":     validate_bool,
-    "macosx.window_mode":      ["system", "tab", "window"],


why was this removed?

EricRLeao1311 · 2023-10-18T14:09:22Z

@ksunden I reverted all of the unrelated changes and followed your sugestion of using "validate_hatch = _validate_hatch_pattern" instead of my previous solution.

melissawm · 2023-10-18T15:13:20Z

I believe the failures here are legitimate, could you take a look and try to fix those @EricRLeao1311 ? You can find more information on the failures here: https://github.com/matplotlib/matplotlib/actions/runs/6559832186/job/17816229759?pr=27108#step:13:1

EricRLeao1311 · 2023-10-18T21:40:14Z

There is another difference between the two validators. While one of them accepts 'X', the other does not. This creates problems and makes it fail tests when we accept the 'X' as a valid hatch. I am removing it from validation, but I believe it would be better to accept it, as the discussion is moving towards greater acceptance for different hatches.

EricRLeao1311 · 2023-10-19T18:58:54Z

@melissawm Could you help me me with what I should do in this case where there are 2 inconsistent tests? One that says 'X' is a valid hatch, while the other says that 'X' is an invalid hatch. (_validate_hatch_pattern() accepts 'X' while validate_hatch() does not) Could I remove the test that checks whether 'X' is invalid given that discussions are moving towards greater acceptance of different hatches? Thanks in advance.

story645 · 2023-11-05T02:54:55Z

@EricRLeao1311 sorry for the delay in getting back to you, do you know why one accepts X and the other doesn't?

EricRLeao1311 · 2023-11-12T19:10:09Z

@story645 the discrepancy arises from different implementation logic. In hatch.py we have "valid_hatch_patterns = set(r'-+|/\xXoO.')" but in rcsetup.py we have "unknown = set(s) - {'\', '/', '|', '-', '+', '', '.', 'x', 'o', 'O'}". I don't think there is a greater reason for one to accept X and the other not. I propose aligning both validators to accept 'X' as discussions lean towards greater acceptance, but for this we need to change test cases.

story645 · 2023-11-12T19:45:21Z

I propose aligning both validators to accept 'X' as discussions lean towards greater acceptance

I agree w/ you on that.

EricRLeao1311 · 2023-11-13T16:13:46Z

Since there are test cases that accept X and that do not accept X, how should I proceed? Can I change the test that does not accept capital X to accept it? If so, how could I do this?

story645 · 2023-11-13T19:16:11Z

Can I change the test that does not accept capital X to accept it? If so, how could I do this?

Sounds good to me? I think you move the X out of the fail and into the success set:

matplotlib/lib/matplotlib/tests/test_rcparams.py

Lines 313 to 317 in 3d56dc4

    
           'success': (('--|', '--|'), ('\\oO', '\\oO'), 
        
                       ('/+*/.x', '/+*/.x'), ('', '')), 
        
           'fail': (('--_', ValueError), 
        
                    (8, ValueError), 
        
                    ('X', ValueError)),

oscargus · 2023-11-14T15:40:36Z

lib/matplotlib/hatch.py

-            _api.warn_deprecated(
-                '3.4',
-                removal='3.9',  # 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.'
-            )


I do not think this should be removed. #20690 is not yet implemented (the idea is to completely rework the hatch handling), so no need to raise an error yet.

EricRLeao1311 added 2 commits October 16, 2023 19:38

github-actions bot reviewed Oct 16, 2023

View reviewed changes

ksunden requested changes Oct 16, 2023

View reviewed changes

melissawm added Maintenance topic: hatch labels Oct 16, 2023

EricRLeao1311 added 4 commits October 17, 2023 10:29

Merge branch 'matplotlib:main' into deduplicate-hatch-validate

659e535

returning the deleted functions

29fd872

returning the deleted functions and refactoring validate_hatch

30a4fc4

Merge branch 'matplotlib:main' into deduplicate-hatch-validate

d3736ad

EricRLeao1311 added 3 commits October 18, 2023 13:50

allowing None and adding return

862d581

raising error when hatch is not string

39eaaed

removing 'X' from valid hatch due to test failure

a1097f9

This was referenced Oct 23, 2023

Stackplot hatching #27169

Closed

Stackplot hatching nbarlowATI/matplotlib#1

Merged

EricRLeao1311 added 2 commits November 13, 2023 13:10

Merge branch 'matplotlib:main' into deduplicate-hatch-validate

9ba660f

Addding 'X' as valid hatch

400ce9d

EricRLeao1311 added 2 commits November 14, 2023 12:14

Merge branch 'matplotlib:main' into deduplicate-hatch-validate

dca91b6

Changing test to accept X as hatch

b44c281

oscargus reviewed Nov 14, 2023

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate hatch validate #27108

Deduplicate hatch validate #27108

EricRLeao1311 commented Oct 16, 2023 •

edited

Loading

github-actions bot left a comment

ksunden left a comment

ksunden Oct 16, 2023

EricRLeao1311 Oct 16, 2023

ksunden Oct 17, 2023

ksunden Oct 16, 2023

ksunden Oct 16, 2023

ksunden Oct 16, 2023

ksunden Oct 16, 2023

ksunden Oct 16, 2023

ksunden Oct 16, 2023

EricRLeao1311 commented Oct 18, 2023

melissawm commented Oct 18, 2023

EricRLeao1311 commented Oct 18, 2023

EricRLeao1311 commented Oct 19, 2023

story645 commented Nov 5, 2023

EricRLeao1311 commented Nov 12, 2023

story645 commented Nov 12, 2023

EricRLeao1311 commented Nov 13, 2023

story645 commented Nov 13, 2023

oscargus Nov 14, 2023

Deduplicate hatch validate #27108

Are you sure you want to change the base?

Deduplicate hatch validate #27108

Conversation

EricRLeao1311 commented Oct 16, 2023 • edited Loading

PR summary

PR checklist

github-actions bot left a comment

Choose a reason for hiding this comment

ksunden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricRLeao1311 commented Oct 18, 2023

melissawm commented Oct 18, 2023

EricRLeao1311 commented Oct 18, 2023

EricRLeao1311 commented Oct 19, 2023

story645 commented Nov 5, 2023

EricRLeao1311 commented Nov 12, 2023

story645 commented Nov 12, 2023

EricRLeao1311 commented Nov 13, 2023

story645 commented Nov 13, 2023

Choose a reason for hiding this comment

EricRLeao1311 commented Oct 16, 2023 •

edited

Loading