-
Notifications
You must be signed in to change notification settings - Fork 78
Add a sort to tables.subset() #2489
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
Conversation
|
@petrelharp: I seem to have opened up a can of worms. This is failing on edge ordering when comparing the tsutil python version of subset() (here) to the new one, even though I added a |
fc50e32 to
c5e5c25
Compare
8082a3e to
09db06f
Compare
OK - fixed now. In Ready for review. |
Codecov Report
@@ Coverage Diff @@
## main #2489 +/- ##
==========================================
- Coverage 93.91% 93.90% -0.01%
==========================================
Files 28 27 -1
Lines 27216 27191 -25
Branches 1263 1263
==========================================
- Hits 25559 25535 -24
+ Misses 1623 1622 -1
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
benjeffery
left a comment
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.
LGTM, one small paranoia point about the testing.
09db06f to
96e74d9
Compare
|
Thanks @benjeffery - changed |
jeromekelleher
left a comment
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.
LGTM, but I think we need to make it clear that there'll be some extra sorting in the tables.subset case, as this could potentially break some code.
96e74d9 to
5ddba5f
Compare
Fixes #2487, see extra discussion in #2473
PR Checklist: