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

TST: restore global state modified during tests #4677

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

yut23
Copy link
Member

@yut23 yut23 commented Sep 27, 2023

PR Summary

Restore numpy's error handling settings and yt's logging level during module teardown.

I found that test_fisheye in yt/visualization/volume_rendering/tests/test_vr_cameras.py was failing when I ran just the visualization tests, but not the full test suite.

PR Checklist

  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

neutrinoceros
neutrinoceros previously approved these changes Sep 27, 2023
Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

good catch, thank you !
here are a couple minor comments but I'd merge it as is

yt/utilities/tests/test_set_log_level.py Show resolved Hide resolved
@@ -5,12 +5,13 @@
from yt.testing import amrspace
from yt.utilities.lib.alt_ray_tracers import _cyl2cart, cylindrical_ray_trace

left_grid = right_grid = amr_levels = center_grid = data = None
left_grid = right_grid = amr_levels = center_grid = data = old_settings = None
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of these all living on the same line. No harm done in keeping the existing one (though I'd also approve expanding it) but I'd rather have the new binding on a separate line.

@neutrinoceros
Copy link
Member

I'd merge it as is

well... when tests pass, I guess 😅

@yut23
Copy link
Member Author

yut23 commented Sep 27, 2023

Huh, I have no idea why only py3.9 is failing. I wasn't able to get test_fisheye to pass locally on 3.10, 3.11, or 3.12.

@neutrinoceros
Copy link
Member

I think I saw this error being flaky a while back in a different context. If I recall correctly this was due to some uninitialised memory being involved in an array division, which may trigger this RuntimeWarning but only sometimes.

@yut23
Copy link
Member Author

yut23 commented Sep 27, 2023

I tried running under valgrind, and the test passed with no memory errors. I'm pretty sure valgrind is just swallowing the FPE signals before numpy can handle them, though; I broke into a pdb session and the image array is all zeros with and without valgrind. I also checked against yt 3.6.1 and got the same error.

Specifically, numpy's error handling settings and yt's logging level.
@yut23 yut23 force-pushed the restore_global_state_in_tests branch from 65d9946 to 3760d58 Compare September 27, 2023 21:30
@yut23 yut23 changed the title Restore global state modified during tests TST: restore global state modified during tests Sep 27, 2023
@yut23
Copy link
Member Author

yut23 commented Sep 27, 2023

Alright, adding log_fields=[False] makes it produce a non-empty image. Should I make a separate PR for that?

@neutrinoceros
Copy link
Member

Should I make a separate PR for that?

I don't think that's necessary.

@yt-fido test this please

@neutrinoceros neutrinoceros added bug tests: running tests Issues with the test setup labels Sep 28, 2023
@neutrinoceros
Copy link
Member

@yt-fido test this please

@neutrinoceros neutrinoceros merged commit 0f84cc2 into yt-project:main Sep 28, 2023
10 checks passed
@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Sep 28, 2023
@yut23 yut23 deleted the restore_global_state_in_tests branch February 8, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug tests: running tests Issues with the test setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants