Skip to content

Don't update sample flags in simplify #2663

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
Jan 13, 2023
Merged

Don't update sample flags in simplify #2663

merged 1 commit into from
Jan 13, 2023

Conversation

jeromekelleher
Copy link
Member

@jeromekelleher jeromekelleher commented Dec 14, 2022 •
edited by benjeffery
Loading

Closes #2662

Stacked on #2619

petrelharp
petrelharp reviewed Dec 14, 2022
View reviewed changes
petrelharp
petrelharp reviewed Dec 14, 2022
View reviewed changes
@petrelharp
Copy link
Contributor

Looks good so far! Needs documentation in python of the "don't update flags" thing, but I imagine you know that.

@jeromekelleher jeromekelleher marked this pull request as draft December 14, 2022 19:18
@jeromekelleher
Copy link
Member Author

Sorry, should have marked this as draft, in the middle of writing it.

Most of what's here is from #2619

@jeromekelleher jeromekelleher mentioned this pull request Dec 15, 2022
Merged
@jeromekelleher jeromekelleher added this to the Python 0.5.4 milestone Jan 8, 2023
@jeromekelleher jeromekelleher mentioned this pull request Jan 8, 2023
Closed
@jeromekelleher jeromekelleher force-pushed the no-update-samples branch 3 times, most recently from 810988d to a84d84e Compare January 10, 2023 13:59
@jeromekelleher jeromekelleher marked this pull request as ready for review January 10, 2023 14:00
@jeromekelleher
Copy link
Member Author

I think this is ready for a look - hopefully complete besides the final copy-paste of the docs into the TreeSequence.simplify.

benjeffery
benjeffery approved these changes Jan 11, 2023
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! One question about the docstring.

hyanwong
hyanwong approved these changes Jan 12, 2023
View reviewed changes
Copy link
Member

@hyanwong hyanwong left a comment

Choose a reason for hiding this comment

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

LGTM

@jeromekelleher
Copy link
Member Author

Updated to reflect review comments. I shuffled the docs around a little bit to deduplicate the docs for TableCollection.simplify and TreeSequence.simplify. It does need some work and an example section though, I would imagine these docs are pretty dense to outsiders.

@benjeffery
Copy link
Member

I think you just need a rebase here to fix the docs.

@benjeffery
Copy link
Member

Hmm, will look into docs fail.

@benjeffery
Copy link
Member

Ah, I thought it was rebased, but isn't. Thank goodness!

@jeromekelleher
Copy link
Member Author

I just tried rebasing again and it said everything is up to date?

@jeromekelleher
Copy link
Member Author

I should probably nuke the cache

@jeromekelleher
Copy link
Member Author

Oh, can't nuke the cache here. Dunno - any ideas @benjeffery?

@benjeffery
Copy link
Member

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jan 13, 2023

rebase

✅ Branch has been successfully rebased

@benjeffery
Copy link
Member

Oh, can't nuke the cache here. Dunno - any ideas @benjeffery?

I nuked it in #2688 which seems to have done the trick.

@benjeffery
Copy link
Member

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jan 13, 2023

rebase

✅ Branch has been successfully rebased

@codecov
Copy link

codecov bot commented Jan 13, 2023 •
edited
Loading

Codecov Report

Merging #2663 (159ff73) into main (159ff73) will not change coverage.
The diff coverage is n/a.

❗ Current head 159ff73 differs from pull request most recent head ce1dee7. Consider uploading reports for the commit ce1dee7 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2663   +/-   ##
=======================================
  Coverage   93.99%   93.99%           
=======================================
  Files          29       29           
  Lines       28041    28041           
  Branches     1578     1578           
=======================================
  Hits        26357    26357           
  Misses       1648     1648           
  Partials       36       36
Flag Coverage Δ
c-tests 92.26% <0.00%> (ø)
lwt-tests 89.05% <0.00%> (ø)
python-c-tests 71.16% <0.00%> (ø)
python-tests 99.01% <0.00%> (ø)

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

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 159ff73...ce1dee7. Read the comment docs.

@benjeffery benjeffery added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jan 13, 2023
@mergify mergify bot merged commit 5ee30fb into tskit-dev:main Jan 13, 2023
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrelharp petrelharp petrelharp left review comments

@hyanwong hyanwong hyanwong approved these changes

@benjeffery benjeffery benjeffery approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Python 0.5.4
Development

Successfully merging this pull request may close these issues.

Add TSK_SIMPLIFY_NO_UPDATE_SAMPLE_FLAGS
4 participants
@jeromekelleher @petrelharp @benjeffery @hyanwong