-
Notifications
You must be signed in to change notification settings - Fork 10
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
Misc fixes tests #22
Misc fixes tests #22
Conversation
Summary: - don't force num_workers=0 for debug - check control_grid instead of warping_field in test_pipeline.py - add test for dataloader with num_workers=2 in test_data.py
There are some remaining warnings in tests, mostly from import handling deprecations internally in lightning when I'm using python3.11 but also when deprecation warning for torch.meshgrid, which made me uncertain if we're using it correctly. See pytorch/pytorch#50276 For square images it shouldn't matter, but I'll have to look at it again to be sure that it works in general. |
So actually looked at why advex test was flaky and at a first glance it shouldn't be (not strict comparisson) rob_acc > cfg.success_threshold but |
Finally no warnings
|
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.
Thanks, looks great! Only thing I noticed is two places where you have a tuple (None,)
as the default for log_every_n_steps
. I assume that's a typo but wasn't sure enough to just change it and merge. (If it's deliberate and I'm missing something, then at least the type annotation should be different). Feel free to merge after dealing with that!
This is mainly two things:
num_workers > 0
#192 is the questionable one. It adds a new dependency to run test-suite and that dependency has questionable maintenance. Additionally, it would be better to just fix test so that it isn't flaky, but I got a bit annoyed and wanted a quick-fix.
Shouldn't need much review. Either cherry-pick only 1 or merge as is.