[MRG] standardize and simplify search, prefetch, gather results by using dataclasses #1955
Conversation
YOU R GENIUS 🤩 |
Codecov Report
@@ Coverage Diff @@
## latest #1955 +/- ##
==========================================
+ Coverage 83.25% 91.42% +8.16%
==========================================
Files 126 96 -30
Lines 13990 9932 -4058
Branches 1913 1946 +33
==========================================
- Hits 11647 9080 -2567
+ Misses 2071 579 -1492
- Partials 272 273 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
except I forgot that |
pardon my lack of knowledge, but: can't you have the code in a method that IS inherited, and just call that method in |
|
@ctb I think this is ready for preliminary review. In particular, I want to make sure everything for I think we could enable additional things (e.g. calculate jaccard/average containment/max containment/ani directly in the search classes (this might be especially helpful for ANI). But gather results make this a bit trickier (need to distinguish btwn results from original vs subtracted search mh), so I wanted to get your thoughts. For example, we could pass in the subtracted minhash to the gatherresults, and calculate everything in the class... |
On a quick skim, this looks beautifully simple and straightforward, and much cleaner than what we were doing before! (And thank you - it must have been tedious as heck 😆 ) This may be famous last words, but I'm not sure you need much more by the way of tests. The search, gather, and prefetch code is thoroughly tested, and while there's always the opportunity for more testing, the consistency checks we have in the code already should be good enough - your added tests here are just nice gravy.
This would definitely be nice and doesn't strike me as hard, just more tedium - I agree with your idea of passing in the subtracted minhash. Other than addressing the remaining missed code coverage and the questions in there, this is a nice PR already and I'd love to see it merged. |
|
well, this turned into a bit of a beast (😅), but it's ready for review @ctb @sourmash-bio/devs |
bluegenes
changed the title
This PR replaces
SearchResult,PrefetchResult,GatherResultnamedtuples with dataclasses, maintaining current csv output. It's now possible to calculate attributes for each result directly within the dataclass. I've tried to replace external computing with this, but there is certainly more optimization that can be done.To facilitate these comparison calculations, this PR also introduces
MinHashComparisondataclasses, (BaseMinHashComparison,FracMinHashComparison, andNumMinHashComparison) which contain properties for all the comparisons we'd like to build when comparing two minhashes, including automatically downsampling to the lower resolution sketch for comparison.It would be great to think about and/or standardize column output a bit more, ref #1555, #1737, but at least now the output is only specified in a single spot, so it should be easier to change. Columns can be easily added to the written output for each class by adding to the
*write_colslist in the class.related:
avg_abundtoavg_covoravg_coverage#1737.motivation: I need to add several columns (and calculations) for ANI estimation, and namedtuples were getting repetitive and unruly.
benefit: Many of the params we're passing into our search results are calculated from the two sketches. We can automate this within the dataclass to avoid needing to write out the calculation each time.