Skip to content

Fixing check for shared history in union #904

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 9, 2020
Merged

Fixing check for shared history in union #904

merged 1 commit into from
Nov 9, 2020

Conversation

@mufernando
Copy link
Member

@mufernando mufernando commented Oct 9, 2020

Union should ignore top-level metadata/schema and provenance tables when checking for shared history equality.

@AdminBot-tskit
Copy link
Collaborator

AdminBot-tskit commented Oct 9, 2020

📖 Docs for this PR can be previewed here

@mufernando mufernando mentioned this pull request Oct 9, 2020
Closed
@mufernando mufernando requested a review from benjeffery October 9, 2020 21:04
@codecov
Copy link

codecov bot commented Oct 9, 2020 •
edited
Loading

Codecov Report

Merging #904 (749e2ae) into main (a1f91f5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #904   +/-   ##
=======================================
  Coverage   93.40%   93.41%           
=======================================
  Files          26       26           
  Lines       20395    20389    -6     
  Branches      825      825           
=======================================
- Hits        19050    19046    -4     
+ Misses       1308     1306    -2     
  Partials       37       37
Flag Coverage Δ
c-tests 92.45% <100.00%> (+0.01%) ⬆️
lwt-tests 93.66% <ø> (ø)
python-c-tests 94.38% <ø> (ø)
python-tests 98.53% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
c/tskit/tables.c 90.66% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1f91f5...749e2ae. Read the comment docs.

@benjeffery
Copy link
Member

benjeffery commented Oct 9, 2020

Great stuff! I'll review after #897 is in and we have a clean diff.

@mufernando
Copy link
Member Author

mufernando commented Oct 23, 2020

Waiting for #917

@mufernando mufernando marked this pull request as draft October 23, 2020 16:42
@jeromekelleher
Copy link
Member

jeromekelleher commented Oct 27, 2020

Probably a good idea to get this one shipped away @mufernando - can you bring it up to date please?

@mufernando mufernando self-assigned this Oct 28, 2020
@mufernando mufernando force-pushed the union_fix_eq branch 2 times, most recently from 64b4655 to fc6ece7 Compare November 6, 2020 19:21
@mufernando mufernando marked this pull request as ready for review November 6, 2020 20:11
@mufernando
Copy link
Member Author

mufernando commented Nov 6, 2020

I think this is done. I don't know why some tests are failing, because locally they all run fine. Also the error is in a test completely unrelated.

@mufernando mufernando requested a review from petrelharp November 6, 2020 20:12
@benjeffery
Copy link
Member

benjeffery commented Nov 6, 2020

I've seen this one fail before - worryingly it seems flakey. Let me look into it!

@benjeffery
Copy link
Member

benjeffery commented Nov 6, 2020

It has passed on a re-run.

benjeffery
benjeffery reviewed Nov 7, 2020
View reviewed changes
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question about the C tests.

petrelharp
petrelharp approved these changes Nov 9, 2020
View reviewed changes
Copy link
Contributor

@petrelharp petrelharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

benjeffery
benjeffery approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Can we get a squash up in here?

@mufernando
Copy link
Member Author

mufernando commented Nov 9, 2020

Done! Thank you all!

@mergify mergify bot merged commit 24e9c21 into tskit-dev:main Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjeffery benjeffery benjeffery approved these changes

@petrelharp petrelharp petrelharp approved these changes

@jeromekelleher jeromekelleher jeromekelleher approved these changes

Assignees

@mufernando mufernando

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mufernando @AdminBot-tskit @benjeffery @jeromekelleher @petrelharp