search

tdaverse / ggtda

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

Merge cran-prep into master #9

Closed rrrlw closed 4 years ago

rrrlw commented 4 years ago

Major change = second vignette (simplicial.Rmd) A few minor changes (see commits below, happy to expand on specific ones as necessary).

Changes are in preparation for submission of ggtda to CRAN. I believe we are holding submission to JOSS until Shiny functionality is added. @corybrunson, would love your feedback on the changes whenever you get time - thanks! Please feel free to edit the cran-prep branch directly as well.

corybrunson commented 4 years ago

@rrrlw the vignette looks good so far. At one point it points out that the different parameters radius (of stat_disk()) and diameter (of stat_vietoris*() and of stat_cech*()) may cause confusion. Would it be better to allow both parameters to be passed to both stats? We could still use different parameters in the vignette for illustrative purposes.

rrrlw commented 4 years ago

That works! I think the uses of radius and diameter are pretty intuitive the way they're set up right now as well, so I'm open to whichever option you prefer.

corybrunson commented 4 years ago

OK cool. The vignette finale is neat! Maybe it could be titled something like "Persistence, Punctuated" since the goal is to use fixed diameters to build intuition about persistence over a continuously varying diameter. : )

rrrlw commented 4 years ago

I like it! If you haven’t already, feel free to update the title.

Any changes that I should make before you merge this? I am unsure if it matters in which order convenience or cran-prep are merged into master; what do you think?

corybrunson commented 4 years ago

I don't think it should matter, except that we remember to reset the 2-dimensional vdiffr test anchors. Are you ready to merge both into master? Want me to do that and handle whatever conflicts arise (none should)?

rrrlw commented 4 years ago

Sure! Should we work on the warnings returned by devtools::test() in master once both branches have been merged?

corybrunson commented 4 years ago

Agreed—are you still getting errors on this branch after updating the vdiffr figures? Either way, i think it's safe to move everything to master and handle things there. I'll do a first pass now.

corybrunson commented 4 years ago

@rrrlw done! I'll leave it to you to close this PR and delete the branches if everything checks out on your end.

rrrlw commented 4 years ago

Fantastic, thank you @corybrunson! All looks good - I will do a last check when I’m at a computer just to make sure.

rrrlw commented 4 years ago

Committed a couple of changes to fix issues in R CMD check, otherwise all looks good! Deleting the cran-prep branch now