Skip to content
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

Constrain "trunc_" classes to output of rtrunc() #97

Closed
wleoncio opened this issue Nov 30, 2022 · 5 comments
Closed

Constrain "trunc_" classes to output of rtrunc() #97

wleoncio opened this issue Nov 30, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@wleoncio
Copy link
Member

wleoncio commented Nov 30, 2022

Several generic functions on https://github.com/ocbe-uio/TruncExpFam/blob/36c134f038f770fa641d57786c8f9ee73330af8a/R/genericFunctions.R dispatch to trunc_-class object, and yet they have arguments like eta, which has a different structure than the output of rtrunc() so it arguably shouldn't be the same class.

One solution would be to require these functions to also have a y argument, whose purpose would just be to retrieve its class for dispatching.

Another solution would be to refactor the whole package to use RC or R6. This would make the rtrunc_ class a lot better structured, but breaks parallelism with the stats functions, because the syntax to call methods would be different.

Connected to the first idea, perhaps eta can be instead added as an attribute of y, so that only y is passed to the generic and eta is retrieved from it. The same would have to be done with the parms argument, but in this case the attribute must not conflict with the real-parameter attribute created during the rtrunc() call.

@wleoncio wleoncio added the enhancement New feature or request label Nov 30, 2022
@wleoncio
Copy link
Member Author

wleoncio commented Nov 30, 2022

The goal is to remove any direct call of methods. The generics should always be called, even within the code.

@wleoncio wleoncio added this to the MVP 1.3.0 milestone Apr 13, 2023
@wleoncio wleoncio self-assigned this Nov 8, 2023
@wleoncio
Copy link
Member Author

wleoncio commented Nov 8, 2023

To simplify the issue, these are the generics that are affected and need intervention:

  • natural2parameters(eta, ...)
  • parameters2natural(parms, ...)
  • getGradETinv(eta, ...)

All other generics are a function of y, which is correctly-structured as trunc_*-class objects.

@wleoncio
Copy link
Member Author

wleoncio commented Nov 8, 2023

Ok, so I tried the first idea of adding y to the generics, and it just breaks everything, so I don't think that's something worth spending much time on.

The idea of reimplementing classes also feels strange right now: R6 would introduce a dependency just for the sake of refactoring. RC would solve this, but then again, now I think using OOP would require unnecessary major restructuring, since each distribution would be a subclass of the truncated family, and perhaps the same things for the parameters.

Since the regular parameters are already written as attributes of the sample (y), what makes most sense to me right now is to just

  • add eta as attribute of y
  • replace y for eta/parm as the argument on parameters2natural()
  • replace y for eta/parm as the argument on natural2parameters()
  • replace y for eta/parm as the argument on getGradETinv()

wleoncio added a commit that referenced this issue Nov 8, 2023
wleoncio added a commit that referenced this issue Nov 8, 2023
* issue-97:
  Minor fix to parameter passing (#97)
  Increment version number to 1.0.1.9008
@rho62
Copy link
Contributor

rho62 commented Nov 10, 2023 via email

@wleoncio
Copy link
Member Author

New idea, much cleaner than the suggestion above:

There's no reason for parameters2natural() and natural2parameters() to have methods for trunc_* classes, since they are not dependent on samples, only on parameters. Therefore, methods such as parameters2natural.trunc_normal() should be replaced with something like parameters2natural.parms_normal(). Thus, we create new parms_* superclasses just for the parameter arguments parms and eta, which are basic named vectors.

wleoncio added a commit that referenced this issue Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants