Skip to content

Fix max_seqlets_subsample bug#60

Open
lhqing wants to merge 2 commits intojmschrei:mainfrom
lhqing:main
Open

Fix max_seqlets_subsample bug#60
lhqing wants to merge 2 commits intojmschrei:mainfrom
lhqing:main

Conversation

@lhqing
Copy link

@lhqing lhqing commented Feb 27, 2025

Hi, thank for the great tool! I found a potential bug here.

The merging_max_seqlets_subsample=300 in main function doesn't pass into the SimilarPatternsCollapser, which cause the pattern merge step still run under max_seqlets_subsample=1000

caenrigen added a commit to caenrigen/tfmodisco-lite that referenced this pull request Jun 3, 2025
@caenrigen
Copy link
Contributor

caenrigen commented Aug 26, 2025

@jmschrei @bytewife I am wondering if you spot this PR? and if there is any relevant problems that this bug might create

I suggest to additionally change the default argument from merging_max_seqlets_subsample=300 to merging_max_seqlets_subsample=1000 such that the bug is fixed and the behaviour of modiscolite remains the same as it used to be (when using default values ofc). Note that there is also a notebook that maybe should be fixed too, it uses SimilarPatternsCollapser with max_seqlets_subsample = 300

[EDIT: ] Suggested changes over here caedc9a

caenrigen added a commit to caenrigen/tfmodisco-lite that referenced this pull request Aug 26, 2025
@jmschrei
Copy link
Owner

Thanks for pointing out this issue. I don't think we have time to consider changes like this right now. Potentially after the next set of ENCODE papers comes out there will be more time. Sorry about that.

@caenrigen
Copy link
Contributor

I understand, thank you for the reply 🙂

caenrigen added a commit to caenrigen/tfmodisco-lite that referenced this pull request Aug 27, 2025
@austintwang
Copy link
Contributor

Hi @caenrigen, tfmodisco-lite has been merged into the official TF-MoDISco repository. We encourage you to re-open your PR there.

caenrigen pushed a commit to caenrigen/tfmodisco that referenced this pull request Oct 6, 2025
caenrigen pushed a commit to caenrigen/tfmodisco that referenced this pull request Oct 6, 2025
caenrigen added a commit to caenrigen/tfmodisco that referenced this pull request Oct 6, 2025
austintwang added a commit to kundajelab/tfmodisco that referenced this pull request Oct 12, 2025
* Fix max_seqlets_subsample bug

Mirror the fix in: jmschrei/tfmodisco-lite#60

* Remove unused trim_min_length argument in report functions

* Update descriptive_report calls

* Changelog

---------

Co-authored-by: Austin Wang <austin.wang1357@gmail.com>
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.

4 participants