Skip to content

Conversation

@Phlya
Copy link
Member

@Phlya Phlya commented Jan 13, 2022

Coverage saves total cis contacts into info (when store=True).
Sample can have total cis contacts as target: tries to use the stored value and calculates it if it's not there.

@Phlya Phlya mentioned this pull request Jan 20, 2022
count=None,
frac=None,
cis_target=False,
exact=False,
Copy link
Member

Choose a reason for hiding this comment

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

consider adding ignore_diags here ? or ultimately dist_min and dist_max ?
if it does not end up oin this PR - leave an issue, maybe ? for the future

Copy link
Member Author

Choose a reason for hiding this comment

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

Would ignore diags only apply to cis_count? Or any count?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say yes ... Also just make sure what ignore_diags means exactly - ignore diags to set the count target, of course - but what should happen with the ignored_diags in the subsamples matrix ? Should they be zero ?

Copy link
Member

Choose a reason for hiding this comment

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

I dunno - maybe leave ignore_diags as an issue to discuss a bit more - but proceed with this one as is ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes meaning ... which of the options?
I'd say the calculation of the subsampling fraction should not take ignore_diags diags into account, but subsampling would be applied to all data including those diags

Copy link
Member Author

Choose a reason for hiding this comment

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

OK! Merge then?

@Phlya
Copy link
Member Author

Phlya commented Mar 31, 2022

All done except ignore_diags, also added tests for coverage API, and fixed a bug in exact sampling (which was broken for me due to wrong dtype).

@Phlya
Copy link
Member Author

Phlya commented Mar 31, 2022

Apart from the idea about ignore_diags this is ready IMO

# Exact sampling is very slow! So commented out
# cooltools.api.sample.sample(
# clr,
# op.join(request.fspath.dirname, "data/CN.mm9.1000kb.test_sampled.cool"),
Copy link
Member

Choose a reason for hiding this comment

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

can we use a smaller input sample to uncomment this ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is our normal test cooler at 1Mb resolution... So quite small! Idk we could add a 10Mb resolution one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with 10Mb resolution it takes a while... This whole file takes 95 sec to run through pytest on my laptop, which I guess is acceptable?

Copy link
Member

Choose a reason for hiding this comment

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

let's try this way - w'd need to optimize out tests at some point anyways

@sergpolly sergpolly merged commit 5641038 into master Apr 1, 2022
@gfudenberg gfudenberg deleted the cis/trans branch August 24, 2022 19:40
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.

3 participants