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

BUG: fix two bugs in yt.visualization.eps_writer #3496

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Sep 1, 2021

PR Summary

Noticed that eps_writer didn't have tests so I ran an example from the documentation and was able to find a trivial but blocking bug.
While testing this I also fixed another bug where save_fig didn't accept Path objects.

PR Checklist

[N/A] New features are documented, with docstrings and narrative docs

  • Adds a test for any bugs fixed. Adds tests for new features.

@neutrinoceros neutrinoceros force-pushed the hotfix_eps_writer branch 3 times, most recently from 5511de2 to d8687fa Compare September 1, 2021 08:01
@neutrinoceros
Copy link
Member Author

Turns out testing this in CI is hard. I'll get back to it.

@neutrinoceros neutrinoceros marked this pull request as draft September 1, 2021 08:31
@jwise77
Copy link
Contributor

jwise77 commented Sep 1, 2021

Thanks for creating this test and fixing the bug. All of the changes you've made so far look good. I can review any other future changes in this PR.

@neutrinoceros
Copy link
Member Author

So I think the reason why CI was failing is that the "tex" executable isn't available on the virtual machines we test with. Unfortunately this means that my test is doomed to never run automatically, which limits greatly its value. On the other hand it seems clear that this module isn't heavily used currently since no body seemed to have noticed it was broken, and it's still better to have a reference test than no tests.

On a broader level, I'm thinking this module likely has applicability far beyond yt and it would probably make sense to migrate it to its own project ? it would also help making sure it's properly tested.
And even before that, I understand that you wrote this functionality out of frustration with matplotlib's own rasterising capabilities, any idea if it got any better since then ?
This discussion goes well beyond the scope of this PR though, maybe we should open a dedicated issue.

@neutrinoceros
Copy link
Member Author

What's important to review this PR is to make sure my test works at least locally.

@neutrinoceros neutrinoceros marked this pull request as ready for review September 1, 2021 12:46
- a call to get_cmap was syntaxically erroneous
- filename argument didn't accept Path objects
Copy link
Contributor

@jwise77 jwise77 left a comment

Choose a reason for hiding this comment

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

This all looks good. Thanks again. I agree that it'll be easier to test this module if it's an extension. I'll make an issue and work on it soon.

@jzuhone jzuhone merged commit b1e3e28 into yt-project:main Sep 3, 2021
@neutrinoceros neutrinoceros deleted the hotfix_eps_writer branch September 3, 2021 19:01
@neutrinoceros neutrinoceros mentioned this pull request Oct 12, 2021
42 tasks
@matthewturk
Copy link
Member

@meeseeksdev backport to yt-4.0.x

@lumberbot-app
Copy link

lumberbot-app bot commented Oct 15, 2021

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout yt-4.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 b1e3e289f2efd0c35a993b6c520ef284b9a6df28
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3496: BUG: fix two bugs in yt.visualization.eps_writer'
  1. Push to a named branch :
git push YOURFORK yt-4.0.x:auto-backport-of-pr-3496-on-yt-4.0.x
  1. Create a PR against branch yt-4.0.x, I would have named this PR:

"Backport PR #3496 on branch yt-4.0.x (BUG: fix two bugs in yt.visualization.eps_writer)"

And apply the correct labels and milestones.

Congratulation you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove Still Needs Manual Backport label once the PR gets merged.

If these instruction are inaccurate, feel free to suggest an improvement.

@neutrinoceros
Copy link
Member Author

@meeseeksdev backport to yt-4.0.x

meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Oct 16, 2021
neutrinoceros added a commit that referenced this pull request Nov 1, 2021
…6-on-yt-4.0.x

Backport PR #3496 on branch yt-4.0.x (BUG: fix two bugs in yt.visualization.eps_writer)
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.

None yet

4 participants