-
Notifications
You must be signed in to change notification settings - Fork 15
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
Package structure and documentation #35
Comments
Please be mindful of all the work that has been done and how the theory has grown. If I would code it today knowing all the papers that have been published by me and others a different structure would have been done. What you are proposing is a complete refactor and breaking for a lot of users. So we have to move slowly and carefully. @astamm changes already have caused heartache across users and I have users who will not upgrade right now. This theory goes from functions in |
Apologies if my changes were breaking changes. I thought I was just adding options with proper defaults so that old code should not break but it seems I have not been careful enough. This advocates in favour of putting into place a proper test suite in order to make sure that PRs do not break code that is supposed to work in a certain way. The current test suite apparently does not achieve this goal since my previous changes passed all the tests. |
Its more that you changed the test suite to match the changes so it was good. Problem was there were people who are using old version and options changed. Its fine I got most of them through it, but what is being proposed here is a lot larger change and we need to be careful. |
I am very sorry that I did that. Ok, if we want to do that, we should agree on a test suite first with expected behaviour of the existing functions. Then, we make and accept changes that do not break the test suite as it is. Maybe we add functions that should replace old ones but we introduce a process of soft deprecation of old functions first, so that test suite does not break. This also leaves time to users to maybe adapt their code as we could make a message appear when they use soft-deprecated functions, suggesting to use a new syntax. The deprecation phase could last a year or something. Would that be a good way of doing things? |
I agree with that, first I would upgrade the tests, then I would make the changes. I am okay not having a soft deprecation, as longs as we just tell the user where the function now lies and how to use it. |
Perfect. Are you happy with the test suite as it is or should it be updated? Once we agree upon it, @araiari and I will work on a PR as time permits. |
I am mostly happy with it, should be expanded though for sure. |
@astamm and I would like to express you two general comments about the package:
Distinction for univariate and multivariate functions
We wonder why the package provide distinct functions to deal with unidimensional functions (e.g.,
elastic.distance()
,f_to_srvf()
, …) and multidimensional functions (e.g.,calc_shape_dist()
,curve_to_q()
, …, always specifyingmode=“O”
,rotation=FALSE
andscale=FALSE
). We believe that this distinction makes the package less clear to external users and increases the complexity in the package maintenance. For instance, we could havecalc_shape_dist()
be an internal function and expose a number of functions to the user for covering various choices of metric spaces. The benefit for the external user would be to have functions with less inputs and probably more specific to their case studies.General improvements
In general, we noticed possible difficulties in the use of the package due to the documentation (with missing indications for input object type and sizes, lack of references, brief descriptions and often no details) and to a possibly non-user-friendly structure of the package and its functions. The package possibly reaches a large audience thanks to the super neat mathematical ideas therein. It would certainly benefit from an improved documentation.
If you want, @astamm and me could give it a go and send a PR on this.
The text was updated successfully, but these errors were encountered: