search

tdaverse / ggtda

ggplot2 extension to visualize persistent homology
https://tdaverse.github.io/ggtda/
GNU General Public License v3.0
20 stars 5 forks source link

split pareto_persistence into separate testable functions #26

Closed corybrunson closed 3 years ago

corybrunson commented 3 years ago

fix(pareto_persistence): rm rPref param, write & test base & rPref functions

NOTE: Instead of using the additional .use_rPref parameter, this commit splits the pareto_persistence function into a base calculator and an rPref calculator, with the original calling one or the other according as rPref is installed. This restores the pre-parameter behavior elsewhere and allows both calculator functions to be tested (provided rPref is installed).

I personally think this solution is better. We don't need 100% coverage, and this commit allows coverage of both pareto calculators without making the underlying code too strange.

This is a sub-branch of coverage, so the PR is directed there. Accepting both PRs will (theoretically) end with this commit in main.

rrrlw commented 3 years ago

In both branches (coverage and coverage2), rPref is in Imports in DESCRIPTION; shouldn't this guarantee that rPref will be installed? (line 198 in persistence.R checks if it's installed)

A side note: if uncertain about whether a package is available, I've seen 2 ways of handling it - first, by checking utils::installed.packages(), as is done here; second, by using requireNamespace(..., quietly = TRUE) as shown here in the Suggests bullet. Any idea which is better or if they're equivalent?

corybrunson commented 3 years ago

Hm. No idea on the second question; clearly i'm new to this sort of concern! But good call on the first. I'll see what happens when i demote rPref—i don't know if checks will fail.

By the way, why is ripserr imported rather than also only suggested?

rrrlw commented 3 years ago

Good catch, I think it'd be better to move ripserr to Suggests as well and check if it's installed w/in vignettes (or wherever it's being used).

corybrunson commented 3 years ago

Actually, i think vignettes can use packages without qualification if they're suggested rather than imported. For example, the "Labeling small strata" vignette in ggalluvial calls ggrepel and ggfittext, both of which are only Suggests in the DESCRIPTION. I believe this does cause issues if you try to build vignettes but don't have these packages installed.

So, if OK with you, i'll just move risperr to Suggests and see if problems arise. Shall we do that in a separate branch after these PRs are resolved?

rrrlw commented 3 years ago

I see, good to know! Yes, that sounds like an appropriate plan. This PR looks good to me, approving changes now, merge at your convenience. Will look at #25 after this is merged. Thanks!

corybrunson commented 3 years ago

Helpfully, when i deleted the coverage branch, the destination of this PR changed from there to main.

corybrunson commented 3 years ago

@rrrlw oh no, i misinterpreted you! I should have merged this commit first. What do you think of the changes just merged in #25 ? It won't be much work to undo them but i know it's now harder to compare them.

corybrunson commented 3 years ago

Here's a comparison of the merge to the most recent commit before it—something i was not sure how to do until just now: https://github.com/rrrlw/ggtda/compare/5b2694d..4397d16

rrrlw commented 3 years ago

All good, just looked over and it looks okay; this one is also good to merge, thanks!