-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add matrix CI Tests on multiple Python versions and OS versions #353
Add matrix CI Tests on multiple Python versions and OS versions #353
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.x #353 +/- ##
=============================================
+ Coverage 56.68% 56.70% +0.02%
=============================================
Files 19 19
Lines 3426 3428 +2
=============================================
+ Hits 1942 1944 +2
Misses 1484 1484
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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.
Very nice addition! Hopefully the fix for windows should be quite straightforward
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.
Nice, this is very useful, @freemansw1 ! Thanks for adding this! And hopefully it will not be to hard to fix the windows issue.
I fixed the Windows bugs through keeping data types constant. Happy to wait for a re-review to merge if you want another look at it @JuliaKukulies @w-k-jones |
I'm happy to merge straight away, I don't think the fix requires a re-review as it looks like a pretty straightforward (and sensible) change |
Actually, as a workflow change, should this be merged into Edit: it has some changes from |
Looks like #293 broke windows compatibility... will need to look into this before merging. |
Could that also be related to the change of dtypes in the feature detection/segmentation dataframes? What was the actual problem before you set these to constant in |
At first glance on my windows machine, it's not a datatype issue... rather the dataframe comes up empty (now, is this itself a result of a data type issue? Perhaps). I will work to debug here. |
On second thought, I'm going to strongly bet that this is a time issue. Ugh. Times are going to be the death of me. |
Okay, this was actually an amazing issue to fix. On Windows, the lines in the test dataset making that are: Line 435 in 5f096e3
result in a float output (with a real <1 value even!). When they are converted to xarray, xarray preserves the milliseconds contained in these values, but our Iris feature detection path does not use them, resulting in incompatible times. This was a neat issue to find actually! Anyway it's resolved now! |
Great job @freemansw1! Do you want to go ahead and merge this? |
This PR adds matrix testing (a matrix of Linux, MacOS, Windows for Pythons 3.8, 3.9, 3.10, 3.11). Right now, this is configured to run on the primary tobac repo (i.e., this one) only, and not on fork repos to save computation time.
Note that this has indicated a bug/issue in testing with
int32
vsint64
datatypes on Windows. I have a windows computer at home; I will have to debug there.