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

Shorthand filter_all argument for simplify #2681

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hyanwong
Copy link
Member

@hyanwong hyanwong commented Jan 12, 2023

Description

Fixes #2607. Changed param name to filter_all to avoid shadowing the builtin python function, although I'm happy to change it to filter and ignore flake8 complaints

PR Checklist:

  • Tests that fully cover new/changed functionality.
  • Documentation including tutorial content if appropriate.
  • Changelogs, if there are API changes.

@hyanwong
Copy link
Member Author

Submitting as a draft for quick look-over. Where should I put the tests?

Fixes tskit-dev#2607. Changed to filter_all to avoid shadowing the builtin python function, although I'm happy to change it to `filter` and ignore flake8 complaints
@jeromekelleher
Copy link
Member

I don't think we can do this in a forward-compatible way without implementing (or at least thinking through) what filter_mutations and filter_migrations might look like.

Let's leave it until after 0.5.4

@hyanwong
Copy link
Member Author

I don't think we can do this in a forward-compatible way...

Yes, good point (link to #20 for reference). Let's leave this open as a template though?

@hyanwong
Copy link
Member Author

I think migrations is less of a problem, because we simply raise an error when simplifying when migrations are present, so guarantees will still hold there. Mutations is certainly an issue though.

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 filter=None argument to simplify
2 participants