Rename APIs for feature density stats and quantitative stats #3618

cmdcolin · 2023-03-30T16:11:18Z

Splits stats fetching into two 'categories': 'quantitative' stats and 'feature density' stats

Adds some back compat. does not rename the rpc method at the moment

cmdcolin · 2023-03-30T16:15:08Z

packages/core/data_adapters/BaseAdapter.ts

@@ -93,7 +93,7 @@ export abstract class BaseAdapter {
  public abstract freeResources(region: Region): void
 }

-export interface Stats {
+export interface FeatureDensityStats {


this could potentially be changed to e.g. {featureDensity:number} | {bytes:number,fetchSizeLimit:number} but that requires additional code changes to make typescript happy, and i think it's ok to leave it as is for now

cmdcolin · 2023-03-30T16:16:05Z

follow up comment to #3605

packages/core/data_adapters/BaseAdapter.ts

codecov · 2023-03-30T17:14:42Z

Codecov Report

Merging #3618 (14bb681) into main (9a9978b) will increase coverage by 0.02%.
The diff coverage is 76.77%.

@@            Coverage Diff             @@
##             main    #3618      +/-   ##
==========================================
+ Coverage   63.06%   63.08%   +0.02%     
==========================================
  Files         871      876       +5     
  Lines       30189    30177      -12     
  Branches     7269     7271       +2     
==========================================
- Hits        19038    19037       -1     
+ Misses      10966    10955      -11     
  Partials      185      185

Impacted Files	Coverage Δ
packages/core/TextSearch/TextSearchManager.ts	`97.05% <ø> (ø)`
packages/core/assemblyManager/assembly.ts	`90.50% <ø> (ø)`
...ments/src/LinearAlignmentsDisplay/models/model.tsx	`80.17% <0.00%> (ø)`
products/jbrowse-img/src/renderRegion.tsx	`59.15% <0.00%> (ø)`
.../src/WiggleRPC/WiggleGetGlobalQuantitativeStats.ts	`7.69% <7.69%> (ø)`
packages/core/data_adapters/BaseAdapter/util.ts	`20.00% <20.00%> (ø)`
plugins/alignments/src/BamAdapter/BamAdapter.ts	`73.45% <50.00%> (ø)`
...ggle/src/MultiLinearWiggleDisplay/models/model.tsx	`66.30% <66.66%> (+2.67%)`	⬆️
...ages/core/data_adapters/BaseAdapter/BaseAdapter.ts	`72.72% <72.72%> (ø)`
...ata_adapters/BaseAdapter/BaseFeatureDataAdapter.ts	`81.66% <81.66%> (ø)`
... and 20 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmdcolin · 2023-03-30T23:56:38Z

one thing I became aware of is that it is a breaking behavior to 'use the new api instead of the old api'

rename estimateStats->getStats on base adapter
add compat so that calling estimateStats on base adapter forwards to getStats
call getStats now
derived classes (e.g. in a plugin) that implement their own estimateStats are now not called

so, we can flip the naming around, and instead keep the core code calling the old thing, with the new getStats naming forwarding to the old one

or, we can just call this a band-aid rip off thing and just roll with it

cmdcolin · 2023-03-30T23:58:15Z

or, have the 'display code' call both the old and new apis to thoroughly check as i think swapping old and new naming leads to same problem too...

cmdcolin · 2023-03-30T23:58:44Z

probably better to just rip off the band aid and not really have back compat if we do go through with renaming maybe

rbuels · 2023-03-31T19:50:53Z

I think I can think of a way to accomplish this renaming without breaking backcompat. How about this plan?

in BaseAdapter
- rename estimateRegionsStats to protected async estimateFeatureDensityStatsWithFeatureSampling
- rename getMultiRegionStats to public async getMultiRegionQuantitativeStats
- rename getRegionStats to public async getRegionQuantitativeStats
- make new public async getRegionFeatureDensityStats that just calls estimateFeatureDensityStatsWithFeatureSampling
- make new public async getMultiRegionFeatureDensityStats that calls getRegionFeatureDensityStats for each region with a Promise.all similar to the implementation of getMultiRegionQuantitativeStats
- make new backcompat deprecated public async estimateRegionsStats that just calls getMultiRegionFeatureDensityStats
- make new backcompat deprecated public async getMultiRegionStats that just calls getMultiRegionQuantitativeStats
- (optional) make new public async getRegionQuantitativeStats that just calls getMultiRegionQuantitativeStats on a single region
in adapter subclasses
- rename any estimateRegionStats implementation to getMultiRegionQuantitativeStats
- rename any getMultiRegionStats to getMultiRegionQuantitativeStats

If I'm not forgetting anything, I think these steps would preserve backcompat while getting us to the point of having a uniform stats API consisting of 4 public methods:

getRegionQuantitativeStats
getMultiRegionQuantitativeStats
getRegionFeatureDensityStats
getMultiRegionFeatureDensityStats

which offer subclasses convenient opportunities for overriding or implementing what makes sense for them.

What do you think of that plan?

cmdcolin · 2023-03-31T20:38:51Z

the backcompat that i am interested in would involve the data adapters not having to rename their methods. I think that trying to add that backcompat may be more effort than it is worth though. It would actually be easier for a plugin, if it is interested in adding that backcompat, changing itself, rather than core adding the code to do any backcompat e.g. a plugin could duplicate the methods in their code by having both estimateRegionsStats and getMultiRegionQuantitativeStats do the same thing if they want the backcompat, rather than core trying to bend around it...

cmdcolin · 2023-03-31T20:45:09Z

note also: as it stands, i do not have a singular version of getRegionFeatureDensityStats in the BaseAdapter API, just getMultiRegionFeatureDensityStats

we could potentially add it, but I am not sure if is a good idea for getMultiRegionFeatureDensityStats to just call the getRegionFeatureDensityStats on every single region. That may result in a lot of time spent doing this feature density stats calculations for multi-region views. The current BaseAdapter code actually just looks at region 0. The index-based-byte-sized estimation from BAM/CRAM actually does incorporate all the regions, because it is fast to do so

rbuels · 2023-03-31T20:49:48Z

the backcompat that i am interested in would involve the data adapters not having to rename their methods

Yes I don't see any way around the data adapter subclasses needing to rename their methods.

At the end of the day, I just want us to have an API for the data adapters that has good naming and can be subclassed by lots of different kinds of data adapters and still have the names make sense. I'm fine with just looking at region 0 in the getMulti* methods

cmdcolin · 2023-04-03T18:49:02Z

this PR now doesn't make any effort to do back compat

reasoning

since the adapter renaming issue cannot be resolved, it doesn't make any effort to do so there
the only other angle is third party code calling the adapter APIs and trying to smooth over this, but i think it's relatively uncommon and is normally proxied through an rpc call, and it would add a fair amount of extra code to deal with it, so I just skip it entirely now

…veStats

cmdcolin · 2023-04-06T02:49:42Z

plugins/wiggle/src/LinearWiggleDisplay/models/model.tsx

-          } catch (e) {
-            self.setError(e)
-          }
+          superReload()


this is a rider change but i manually tested it and our integration tests also test it, don't need to manually do stats as autorun will do so after clearing the error

cmdcolin · 2023-04-06T02:49:50Z

plugins/wiggle/src/MultiLinearWiggleDisplay/models/model.tsx

-          } catch (e) {
-            self.setError(e)
-          }
+          superReload()


similar here

cmdcolin · 2023-04-11T22:07:21Z

maybe can merge for now :) note @rbuels this may end up getting conflicts with your branch since it moves base adapter files around, one file per class

rbuels · 2023-04-11T22:10:30Z

sure that's fine

…

On Tue, Apr 11, 2023 at 3:07 PM Colin Diesh ***@***.***> wrote: maybe can merge for now :) note @rbuels <https://github.com/rbuels> this may end up getting conflicts with your branch since it moves base adapter files around, one file per class — Reply to this email directly, view it on GitHub <#3618 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAASAFMCT2A4YTAES5L2X23XAXI2HANCNFSM6AAAAAAWNQQULU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 30, 2023

cmdcolin commented Mar 30, 2023

View reviewed changes

cmdcolin added internal and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Mar 30, 2023

cmdcolin force-pushed the clarify_adapter_apis branch 3 times, most recently from c51140b to a7248c9 Compare March 30, 2023 16:48

rbuels reviewed Mar 30, 2023

View reviewed changes

packages/core/data_adapters/BaseAdapter.ts Outdated Show resolved Hide resolved

cmdcolin force-pushed the clarify_adapter_apis branch from 4ee265c to 1c56973 Compare March 31, 2023 23:31

cmdcolin added 5 commits April 5, 2023 20:47

Start

9b42b0b

More changes

0eb77b0

Rename updateStats,FeatureStats to updateQuantitativeStats,Quantitati…

334a6ef

…veStats

Update docs

0886910

Update docs

a6f5f5c

cmdcolin force-pushed the clarify_adapter_apis branch from 1c56973 to 391cc4b Compare April 6, 2023 02:48

cmdcolin commented Apr 6, 2023

View reviewed changes

cmdcolin force-pushed the clarify_adapter_apis branch from 391cc4b to c2e0c95 Compare April 6, 2023 02:50

Updates

14bb681

cmdcolin force-pushed the clarify_adapter_apis branch from c2e0c95 to 14bb681 Compare April 6, 2023 02:56

cmdcolin merged commit 29e641f into main Apr 11, 2023

cmdcolin changed the title ~~Clarify adapter APIs for stats fetching endpoints~~ Rename APIs for feature density stats and quantitative stats Apr 11, 2023

cmdcolin deleted the clarify_adapter_apis branch April 11, 2023 22:33

cmdcolin mentioned this pull request Apr 14, 2023

v2.5.0 release announcement #3643

Merged

cmdcolin added a commit that referenced this pull request May 18, 2023

Rename APIs for feature density stats and quantitative stats (#3618)

fae77c4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename APIs for feature density stats and quantitative stats #3618

Rename APIs for feature density stats and quantitative stats #3618

cmdcolin commented Mar 30, 2023

cmdcolin Mar 30, 2023

cmdcolin commented Mar 30, 2023

codecov bot commented Mar 30, 2023 •

edited

Loading

cmdcolin commented Mar 30, 2023

cmdcolin commented Mar 30, 2023

cmdcolin commented Mar 30, 2023

rbuels commented Mar 31, 2023

cmdcolin commented Mar 31, 2023

cmdcolin commented Mar 31, 2023

rbuels commented Mar 31, 2023

cmdcolin commented Apr 3, 2023

cmdcolin Apr 6, 2023

cmdcolin Apr 6, 2023

cmdcolin commented Apr 11, 2023

rbuels commented Apr 11, 2023 via email

Rename APIs for feature density stats and quantitative stats #3618

Rename APIs for feature density stats and quantitative stats #3618

Conversation

cmdcolin commented Mar 30, 2023

cmdcolin Mar 30, 2023

Choose a reason for hiding this comment

cmdcolin commented Mar 30, 2023

codecov bot commented Mar 30, 2023 • edited Loading

Codecov Report

cmdcolin commented Mar 30, 2023

cmdcolin commented Mar 30, 2023

cmdcolin commented Mar 30, 2023

rbuels commented Mar 31, 2023

cmdcolin commented Mar 31, 2023

cmdcolin commented Mar 31, 2023

rbuels commented Mar 31, 2023

cmdcolin commented Apr 3, 2023

cmdcolin Apr 6, 2023

Choose a reason for hiding this comment

cmdcolin Apr 6, 2023

Choose a reason for hiding this comment

cmdcolin commented Apr 11, 2023

rbuels commented Apr 11, 2023 via email

codecov bot commented Mar 30, 2023 •

edited

Loading