-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Preserve variables order in Dataset.concat() #1049
Conversation
data_vars = list(ds.data_vars.keys()) | ||
self.assertEqual(data_vars, data_vars_ref) | ||
# coords are now at the end of the list, so the test below fails | ||
# self.assertEqual(all_vars, all_vars_ref) |
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.
Actually, I think this test passes in this version?
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.
It still doesn't pass on my linux machine :(
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.
Indeed, concat does not currently preserve the order of all variables (yet). But at least we can make it non-stochastic.
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.
You could probably do this by consolidating the two loops over all variables. But up to you if you want to go through the trouble.
Agreed, this is the better approach. Thanks for diving into this! |
Alternatively, it looks like the issue is on line 266 where we iterate
This has the advantage of preserving the set, which is important if there is a very large number of variables. |
Ok, done. The second problem about the coordinate variable seems trickier to solve though... |
@shoyer could you elaborate on that? If there's a way to make it right I am happy to try, but I'm afraid I'm not sure what is the way to go. |
OK, I'm just going to merge this. We can iterate on this more later. |
Alternative (and much easier) implementation to #1048
The problem with the coordinate variables remains the same