-
Notifications
You must be signed in to change notification settings - Fork 51
Swap dims test and value test in == #1111
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
base: main
Are you sure you want to change the base?
Conversation
|
Can you add a similar change for dimstacks? |
|
I added the same change also for DimStacks and also added some tests for it. |
|
This now also fixes isequal by calling isequal on the dims and parent and layerdims for DimStack. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1111 +/- ##
=======================================
Coverage 86.90% 86.91%
=======================================
Files 55 55
Lines 5338 5340 +2
=======================================
+ Hits 4639 4641 +2
Misses 699 699 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| da2 = DimArray(a, (X(2:3), Y(2:3))) | ||
| ds1 = DimStack(da1) | ||
| ds2 = DimStack(da2) | ||
| @test (ds1 == ds2) == false |
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.
I know that this follows directly from the evaluation order, but somehow it feels a bit strange that this should return false and not missing? What do you think?
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.
I think this makes only sense to return false since this should also return false if the parent array is the same.
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.
Yea on second thought this is analogous to the changes to broadcast that I proposed where we should do dims checking before doing any calculations so I think this is the right way to go
| @testset "isequal and == with missing" begin | ||
| a = [missing 1; 1 1] | ||
| da1 = DimArray(a, (X(1:2), Y(1:2))) | ||
| da2 = DimArray(a, (X(2:3), Y(2:3))) |
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.
| da2 = DimArray(a, (X(2:3), Y(2:3))) | |
| da2 = DimArray(copy(a), (X(2:3), Y(2:3))) |
Just to make sure we don't hit any === fallbacks
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.
Good point, should we use deepcopy?
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.
I think copy would be sufficient since a is just an array
This fixes the first half of #1107
This also adds a test for isequal which is currently broken because it goes into the fallback which doesn't take the dimensions into account.
This is the behaviour with this PR and a comparison to the base behaviour: