Skip to content
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

Don't update sample flags in simplify #2663

Merged
merged 1 commit into from Jan 13, 2023

Conversation

jeromekelleher
Copy link
Member

@jeromekelleher jeromekelleher commented Dec 14, 2022

Closes #2662

Stacked on #2619

c/tskit/tables.h Outdated Show resolved Hide resolved
python/tskit/tables.py Outdated Show resolved Hide resolved
@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 added this to the Python 0.5.4 milestone Jan 8, 2023
@jeromekelleher jeromekelleher mentioned this pull request Jan 8, 2023
@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.

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.

python/tests/test_highlevel.py Show resolved Hide resolved
python/tskit/tables.py Outdated Show resolved Hide resolved
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

c/tskit/tables.h Outdated Show resolved Hide resolved
python/tskit/tables.py Outdated Show resolved Hide resolved
@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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TSK_SIMPLIFY_NO_UPDATE_SAMPLE_FLAGS
4 participants