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

Refactoring snipping & pileups #227

Open
2 tasks done
Tracked by #312
agalitsyna opened this issue Mar 30, 2021 · 3 comments
Open
2 tasks done
Tracked by #312

Refactoring snipping & pileups #227

agalitsyna opened this issue Mar 30, 2021 · 3 comments

Comments

@agalitsyna
Copy link
Member

agalitsyna commented Mar 30, 2021

  • The major problem with pileups is that the pileup function does not know about regions and takes them from the input features dataframe.

  • The creation of the "region" column of features requires the user to run "assign_regions", which is a potentially obsolete and unnecessary step (see assign_regions needs to be fixed or removed #225).

  • An alternative is to take the regions directly from snipper object, but the pileup takes only its methods. Should we replace passing two functions with a single snipper instead? If not, why?

@gfudenberg
Copy link
Member

was this fixed @Phlya ? or should we convert to discussion?

@Phlya
Copy link
Member

Phlya commented Nov 4, 2021

There will be a more user friendly API available imminently. So e.g. point 2 will be done internally. However the whole underlying Snipper API is not the most convenient and should be eventually replaced with perhaps dask? @golobor

Maybe we should convert the issue to a discussion as you suggest though! Since we need to think about low-level API design.

@gfudenberg gfudenberg mentioned this issue Nov 17, 2021
49 tasks
@Phlya Phlya mentioned this issue Sep 8, 2022
@gfudenberg
Copy link
Member

gfudenberg commented Sep 14, 2022

update 9/14/2022:

  • create a base class for snipper and re-use its methods.

    • this is because currently, adding modifications to some snipper (e.g., ExpectedSnipper) is already painful if you want to propagate the changes to other snippers.
  • convert bed-like inputs to bedpe at the beginning of pileup and use that internally throughout. this can simplify code in multiple places. e.g.

    region1 = region2 = support

    • this will also remove the need for the assign_view_auto, since we would always assign_view_paired

@gfudenberg gfudenberg changed the title Ideas for the future of snipping and pileups Refactoring snipping & pileups Sep 14, 2022
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

No branches or pull requests

3 participants