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

Fix LaTeX error due to labels that become empty in simplify_labels #9991

Merged
merged 1 commit into from Mar 20, 2020

Conversation

lpsinger
Copy link
Contributor

Fixes #8004.

@pllim
Copy link
Member

pllim commented Feb 27, 2020

Thanks, @lpsinger ! Any chance we can add a test for this use case too?

@lpsinger
Copy link
Contributor Author

I added a unit test, but it is failing with this error message:

RuntimeError: Failed to process string with tex because latex could not be found

Are there any CI jobs that run in an environment with LaTeX installed? Is there already a decorator or a pytest mark that I can use to skip this test if LaTeX is not installed?

@pllim
Copy link
Member

pllim commented Mar 12, 2020

Hmm. Not that I know of. You can try using pytest.importorskip feature to skip it.

@lpsinger
Copy link
Contributor Author

I am using checkdep_usetex from Matplotlib now. See this example:

https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/tests/test_usetex.py#L11-L12

@pllim
Copy link
Member

pllim commented Mar 12, 2020

Might need to skip it regardless if matplotlib version is too old. The job with Matplotlib 2.1.2 is failing. FYI.

@lpsinger
Copy link
Contributor Author

Done.

@lpsinger
Copy link
Contributor Author

Now it's failing with:

NameError: name 'TEX_MISSING' is not defined

I've no idea why, since it is not having a problem with MATPLOTLIB_LT_21.

@lpsinger lpsinger force-pushed the issue8004 branch 4 times, most recently from c17581c to 045ec8f Compare March 12, 2020 21:26
@lpsinger
Copy link
Contributor Author

I don't think that it's even respecting @pytest.mark.skipif('MATPLOTLIB_LT_21'). Something very strange and counterintuitive is going on. I use pytest.mark.skipif all the time, and this behavior does not make sense to me. Help!

@pllim
Copy link
Member

pllim commented Mar 13, 2020

I wonder if it is incompatible with @ignore_matplotlibrc. I will try to push a commit to your PR and see. I hope you don't mind.

@pep8speaks
Copy link

pep8speaks commented Mar 13, 2020

Hello @lpsinger 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-03-19 20:55:04 UTC

@pllim

This comment has been minimized.

@lpsinger
Copy link
Contributor Author

I will try to push a commit to your PR and see. I hope you don't mind.

Not at all. Please go ahead.

@pllim
Copy link
Member

pllim commented Mar 13, 2020

Failures are unrelated but we're also blocked by #10039

@@ -93,6 +93,8 @@ def simplify_labels(self):
self.text[axis][i] = self.text[axis][i][start:]
if starts_dollar:
self.text[axis][i] = '$' + self.text[axis][i]
if self.text[axis][i] == '$$':
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 empty maths block right i.e. $...$ where there is nothing in between, as opposed to the LaTeX for display maths?

Could you add a comment about this just to make it more obvious when reading it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@lpsinger
Copy link
Contributor Author

@pllim, you're right, @ignore_matplotlibrc does not work with skipif. I think that ignore_matplotlibrc would be better implemented as a pytest fixture. I'll put together a patch for that so that we don't have to work around it.

lpsinger added a commit to lpsinger/astropy that referenced this pull request Mar 17, 2020
This allows it to be composed with pytest marks (e.g.
`pytest.mark.skipif`). See astropy#9991.
bsipocz pushed a commit to lpsinger/astropy that referenced this pull request Mar 19, 2020
This allows it to be composed with pytest marks (e.g.
`pytest.mark.skipif`). See astropy#9991.
@bsipocz
Copy link
Member

bsipocz commented Mar 19, 2020

@lpsinger - since your other PR has been merged, could you rebase this and use the new fixture here? Thanks!

@lpsinger
Copy link
Contributor Author

@lpsinger - since your other PR has been merged, could you rebase this and use the new fixture here? Thanks!

Done.

@pllim pllim added the 💤 merge-when-ci-passes Do not use: We have auto-merge option now. label Mar 19, 2020
Copy link
Member

@pllim pllim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bsipocz
Copy link
Member

bsipocz commented Mar 20, 2020

Thank you @lpsinger!

@bsipocz bsipocz merged commit eb9445e into astropy:master Mar 20, 2020
@lpsinger lpsinger deleted the issue8004 branch March 20, 2020 03:25
bsipocz added a commit that referenced this pull request Mar 22, 2020
Fix LaTeX error due to labels that become empty in simplify_labels
bsipocz added a commit that referenced this pull request Mar 22, 2020
Fix LaTeX error due to labels that become empty in simplify_labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TickLabels.simplify_labels corrupts LaTeX label strings on Mollweide axes
5 participants