-
Notifications
You must be signed in to change notification settings - Fork 6
add parameter to control output class #46
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not starting the deprecation right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that users would get a soft notice (introduction of options), then a soft warning (change of default), then a hard warning (deprecation). But i concede that i err toward being too gradual.
R/cubical.R
Outdated
| #' @rdname cubical | ||
| #' @export cubical | ||
| #' @return `PHom` object | ||
| #' @return `"PHom"` or `"persistence"` object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could have the documentation here link to the documentation of as_persistence() and as_PHom().
R/vietoris_rips.R
Outdated
| #' @rdname vietoris_rips | ||
| #' @export vietoris_rips | ||
| #' @return `PHom` object | ||
| #' @return `"PHom"` or `"persistence"` object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this and the above comment, i lean toward non-duplication of hyperlinks, and i believe (or at least intended) that the persistence class is linked in the Details. But i would not object to this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On reflection, i'm with you.
R/vietoris_rips.R
Outdated
| #' <doi:10.1527/tjsai.D-G72>. Persistent homology of the resulting matrix is | ||
| #' then calculated. | ||
| #' | ||
| #' @importFrom phutil as_persistence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to call phutil::as_persistence() than to actually import the function from {phutil}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. Feel free to make that change!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, i'm on it.
|
@astamm could you review again? In particular, wrapping the soft-deprecation call in a helper function undermined its environment-sensitive design, so i dropped it into every place |
This PR introduces the
return_classparameter tovietoris_rips()andcubical(). It defaults to"PHom"and can be set to"persistence"instead; for the latter option, {phutil} becomes a dependency. ThePHom(and default) output should be unchanged, while thepersistenceoutput provides some metadata that the class understands.The plan is to change the default to
"persistence"in the next major release and eventually deprecate the"PHom"class and its option.