search

jdtuck / fdasrvf_R

Functional Data Analysis using Square-Root Slope Framework
10 stars 13 forks source link

Package structure and documentation #35

Open araiari opened 6 months ago

araiari commented 6 months ago

@astamm and I would like to express you two general comments about the package:

  1. 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 specifying mode=“O”, rotation=FALSE and scale=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 have calc_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.

  2. 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.

jdtuck commented 6 months ago
  1. Comments noted, there is history here for a lot users who are used to those functions. We can start making it more integrated and I welcome help. You guys should note this package grew as the theory grew and has been research code.
  2. You can put a PR and I will review if you feel this could be updated. I am a team of one and don't have time to make all these changes.

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 $\mathbb{R}^1$, functions in $\mathbb{R}^n$ (which includes what some call multidimensional functional data, open, and closed curves), and images. If we are going to make changes it needs to be coherent across the theory and not just meeting your goal of unidimensional and multidimensional.

astamm commented 6 months ago

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.

jdtuck commented 6 months ago

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.

astamm commented 6 months ago

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?

jdtuck commented 6 months ago

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.

astamm commented 6 months ago

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.

jdtuck commented 6 months ago

I am mostly happy with it, should be expanded though for sure.