-
Notifications
You must be signed in to change notification settings - Fork 280
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
Checking if extrema are all None in PhasePlot; fixes #3431 #3432
Conversation
I don't understand why the test was canceled? |
Me neither, let's rerun it in hope it was due to a temporary failure from the environment. |
Co-authored-by: Clément Robert <cr52@protonmail.com>
Thanks, @neutrinoceros, for the simplified logic. I wasn't aware of that function, but it's useful. |
I wasn't either ! I usually check more_itertools' doc only when I find myself or others writing this kind of custom logic :) |
I report that this patch doesn't fix the other error raised from your test that I see on my machine. Can you please add it as an actual test here ? |
I added a test, which fails on previous builds but works with this one. I think this should be ready to go assuming the tests pass. Thanks for the help, team. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So as discussed in the issue thread, this other error was my mistake. I confirm that this fixes the bug addressed, and that the test works as intended (red on main, green with this patch).
I found one small issue with how you implemented your test, but otherwise LGTM
Oh, I was worried about the temp file, which is why I implemented this within the superclass of |
Sorry I completely missed that. In this case I approve of this going in as is ! Thank you ! |
@meeseeksdev backport to yt-4.0.x |
PR Summary
Attempts to fix Issue #3431 by checking if
extrema
values are allNone
when using aPhasePlot
, and if so, treats as thoughextrema
itself isNone
. See description in #3431 notes for more details.