Fix minus signs in WCSAxes in TeX mode

[svank]

Description

This is a small fix to #15902. For a moment in that PR, repeated minus signs were trimmed from axis labels, and that still happens when using matplotlib's TeX mode (note the bottom y tick):

from astropy.wcs import WCS
import matplotlib.pyplot as plt
import numpy as np
wcs = WCS(naxis=2)
wcs.wcs.ctype = 'RA', 'DEC'
wcs.wcs.crpix = 25, 25

plt.rcParams['text.usetex'] = True
ax = plt.subplot(1, 1, 1, projection=wcs)
plt.imshow(np.zeros((50, 50)))

This is because minuses are still hyphens in TeX mode (they should be, because LaTeX will render them right, and it happens because formatter_locator._fix_minus only looks at the first character of each label, which is $ in TeX mode). But TickLabels.simplify_labels still expects a unicode minus, so it trims the hyphen.

This PR has TickLabels.simplify_labels expect hyphens in TeX mode, no matter the unicode_minus setting.

I added a test, but it's awkward because TeX tests are currently not run, pending #13326. If I force-enable TeX tests, the new test passes with this fix and fails without this fix:

=================================== FAILURES ===================================
__________________ test_simplify_labels_minus_sign[True-True] __________________

ignore_matplotlibrc = None, usetex = True, unicode_minus = True

    @pytest.mark.parametrize("usetex", [True, False])
    @pytest.mark.parametrize("unicode_minus", [True, False])
    def test_simplify_labels_minus_sign(ignore_matplotlibrc, usetex, unicode_minus):
        with mpl.rc_context(rc={"text.usetex": usetex, "axes.unicode_minus": unicode_minus}):
            ticklabels = TickLabels(None)
            expected_labels = []
            for i in [1, 2]:
                if usetex:
                    label = f"$-{i}$"
                elif unicode_minus:
                    label = f"\N{MINUS SIGN}{i}"
                else:
                    label = f"-{i}"
                ticklabels.add(axis='axis', world=0, angle=0, text=label, axis_displacement=0, data=(0, 0))
                expected_labels.append(label)
            ticklabels.simplify_labels()
>           assert ticklabels.text['axis'] == expected_labels
E           AssertionError: assert ['$-1$', '$2$'] == ['$-1$', '$-2$']
E             
E             At index 1 diff: '$2$' != '$-2$'
E             Use -v to get more diff

tests/test_misc.py:520: AssertionError
=========================== short test summary info ============================
FAILED tests/test_misc.py::test_simplify_labels_minus_sign[True-True] - AssertionError: assert ['$-1$', '$2$'] == ['$-1$', '$-2$']
=================== 1 failed, 33 passed, 1 skipped in 1.93s ====================

[github-actions]

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

• Do the proposed changes actually accomplish desired goals?
• Do the proposed changes follow the Astropy coding guidelines?
• Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
• Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
• Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
• Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
• Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
• Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
• At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

[neutrinoceros]

Thank you for catching and fixing this and sorry I didn't think more about tex mode in my PR. I would expect the new test to live next to the one I added in #15902, or even better, extend the existing test through pytest.mark.parametrize. Any reason not to ?
(note that I'm currently working on fixing my own system where I just found that TeX is broken, so I couldn't run the test yet, but I'll report back)

[svank]

No worries! I wasn't quite sure where to test this, but I guess I thought this test was covering TickLabels.simplify_labels whereas your new test was covering FormatterLocator.formatter---sort of unit testing different parts of the overall feature, so that's why I did it separately. I'm open to other ideas.

[neutrinoceros]

No that's okay, you're right that you're testing a different bit of code, so it seems correct to have a separate test in hindsight. I was able to reprod and confirm that your patch fixed the problem locally. I'll do a detailed review now.
Out of curiosity, do you have any idea why, in your example, the minus sign is missing from the "-20°" label but not from the "-10°" one ?

[neutrinoceros]

I know we're not going to look at these labels, so overlaps don't really matter, but it's still bugging me. Can we make data depend on i ?

[neutrinoceros]

I would prefer we avoid ifs in the body of the test when we can use parametrization instead. What I'm suggesting here is to use a single @pytest.mark.parametrize decorator with three parameters: usetex, unicode_minus, and expected_labels. It be less dynamic but would isolate the test logic from the input/expectation definition.

[neutrinoceros]

does this line also need to happen within the mpl.rc_context ?

[svank]

Out of curiosity, do you have any idea why, in your example, the minus sign is missing from the "-20°" label but not from the "-10°" one ?

I think it's just coming from that function's role in separating out the common prefix and the varying part of each tick label. Without putting the right kind of minus sign in skippable_chars, the minus sign gets shown for the first tick that has it, and then trimmed from successive ones.

Thanks for your review comments! I should have addressed them all.

[neutrinoceros]

I think it's just coming from that function's role in separating out the common prefix and the varying part of each tick label. Without putting the right kind of minus sign in skippable_chars, the minus sign gets shown for the first tick that has it, and then trimmed from successive ones.

thanks a bunch for this detailed explanation ! I think that this would be more obvious if they were more than 2 labels, so I suggest extending the test (it's a loop anyway) to use 4 or 5 of them.
After this round of review I think we'll be all set. Thank you !

[neutrinoceros]

actually I expect this line would be the only one that needs to happen within the rc_context, or am I missing something ?

[svank]

No, you're right!

[neutrinoceros]

Suggested change

	for i in [1, 2]:
	for i in range(1, 6):

[svank]

Alright, I've updated the test again. Thanks for reviewing!

[neutrinoceros]

LGTM now, thanks again !