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

Add stats estimation to JB2 #2497

Merged
merged 31 commits into from
Nov 30, 2021
Merged

Add stats estimation to JB2 #2497

merged 31 commits into from
Nov 30, 2021

Conversation

peterkxie
Copy link
Contributor

@peterkxie peterkxie commented Nov 4, 2021

Add general and specific stats estimation to JBrowse 2, calculates the feature density to potentially limit the loading of tracks. BAM and Tabix files use the index to calculate their feature density

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Nov 4, 2021
@peterkxie peterkxie added discuss in meeting enhancement New feature or request and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Nov 9, 2021
@peterkxie
Copy link
Contributor Author

want to discuss timing issues in meeting, stats estimation makes a lot of tests flaky since it takes extra time, solution seems to be increasing timeout but i'd be increasing a lot of test timeouts

@peterkxie peterkxie marked this pull request as ready for review November 16, 2021 17:11
@cmdcolin
Copy link
Collaborator

can you link the demos showing how this works and provide some analysis of it's behavior on various files?

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #2497 (6eb48ef) into main (abedfa8) will increase coverage by 0.16%.
The diff coverage is 81.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2497      +/-   ##
==========================================
+ Coverage   60.70%   60.86%   +0.16%     
==========================================
  Files         547      547              
  Lines       25579    25732     +153     
  Branches     6042     6080      +38     
==========================================
+ Hits        15528    15663     +135     
- Misses       9725     9743      +18     
  Partials      326      326              
Impacted Files Coverage Δ
packages/core/util/stats.ts 96.00% <ø> (ø)
...ments/src/LinearAlignmentsDisplay/models/model.tsx 69.66% <0.00%> (ø)
...alignments/src/LinearPileupDisplay/configSchema.ts 100.00% <ø> (ø)
...lugins/alignments/src/LinearPileupDisplay/model.ts 63.58% <ø> (ø)
...rc/LinearSNPCoverageDisplay/models/configSchema.ts 100.00% <ø> (ø)
plugins/bed/src/BedTabixAdapter/BedTabixAdapter.ts 76.92% <0.00%> (-6.42%) ⬇️
...earDisplay/models/baseLinearDisplayConfigSchema.ts 100.00% <ø> (ø)
...genome-view/src/LinearBasicDisplay/configSchema.ts 100.00% <ø> (ø)
plugins/wiggle/src/WiggleRPC/rpcMethods.ts 54.05% <0.00%> (ø)
products/jbrowse-img/src/renderRegion.js 0.00% <0.00%> (ø)
... and 14 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 abedfa8...6eb48ef. Read the comment docs.

@cmdcolin
Copy link
Collaborator

volvox filtered vcf track

the volvox filtered vcf track doesn't have very many features...i think ideally, it would not hit any feature density limit?

@cmdcolin
Copy link
Collaborator

here is the link to make the feature density calculation proportional to current zoom level https://github.com/GMOD/jbrowse/blob/dev/src/JBrowse/View/Track/CanvasFeatures.js#L260-L261

the "scale" is equal to 1/bpPerPx

@rbuels
Copy link
Contributor

rbuels commented Nov 17, 2021

I'm betting the tests won't have to do as many clicks of "Force" now that it's accounting for scale

…ar notReady from baselineardisplaymodel, remove super.notReady calls, increase a few flaky test timeouts by a little bit
@cmdcolin
Copy link
Collaborator

if I turn on a bam or cram track e.g. viewing the entirety of chr1 it tries to download the entirety of chr1 and crashes

here it is for bam. the pileup subtrack appears to succeed but the snpcoverage track tries to end up downloading stuff anyways. i think for cram, it does not have e.g. the linecount heuristic and tries to download the entirety of chr1 for both snpcov and pileup

Screenshot from 2021-11-22 16-24-44

@cmdcolin
Copy link
Collaborator

might suggest putting in draft a bit more, just for more testing and such

@cmdcolin cmdcolin marked this pull request as draft November 22, 2021 22:12
@peterkxie
Copy link
Contributor Author

ill look into this more, thanks for the details + screenshot

@peterkxie peterkxie marked this pull request as ready for review November 30, 2021 21:14
@rbuels rbuels merged commit cbfcd97 into main Nov 30, 2021
@rbuels rbuels deleted the 607_stats_estimation_v2 branch November 30, 2021 21:31
cmdcolin added a commit that referenced this pull request Dec 8, 2021
with confirmation from @peterkxie

This reverts commit cbfcd97, reversing
changes made to 9ba6b7b.
@cmdcolin cmdcolin restored the 607_stats_estimation_v2 branch December 8, 2021 23:44
cmdcolin added a commit that referenced this pull request Dec 9, 2021
@cmdcolin cmdcolin deleted the 607_stats_estimation_v2 branch December 9, 2021 23:05
@cmdcolin cmdcolin removed the enhancement New feature or request label Dec 13, 2021
cmdcolin added a commit that referenced this pull request Jan 26, 2022
* add estimateStatsCache logic to other adapters to prevent extra stats calculations

* prevent rpc call if stats already been calculated to stop additional reload

* Remove statsStatus

* Add currentFeatureScreenDensity getter

* fix test and snapshot issues for statsStatus removal change

* Revert "Revert "Merge pull request #2497 from GMOD/607_stats_estimation_v2","

This reverts commit e365b6f.


* Encode the concept of notReady more generically instead of separate check for statsNotReady

* Central bytesForRegions function

* Rename method estimateGlobalStats->estimateRegionStats

* Don't show limit if below 20kb viewing region size

* Disable stats estimation on sequence and bigwig adapters

* Rename to fetchSizeLimit and only store current bpPerPx

* Use simple variables instead of nested model for statsLimit, restores the userBpPerPx field on main

* Remove the byte-size estimation from bed/gff/vcf tabix adapters

* Add zoom in and force load tests

Co-authored-by: peterkxie <peterkxie@yahoo.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.

None yet

3 participants