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

Rethinking the structure of model types #505

Open
ararslan opened this issue Oct 25, 2022 · 0 comments
Open

Rethinking the structure of model types #505

ararslan opened this issue Oct 25, 2022 · 0 comments

Comments

@ararslan
Copy link
Member

I have a local branch where I've been playing around with a fairly massive overhaul of the internals with the following design goals in mind:

  • Allow arrays used during the fitting process to be freed once the model has been fit. On my branch, I was doing this by defining a Workspace type with methods that allocate storage appropriately for different model structures. A workspace is defined at fit time and passed to various functions but is not stored in the model object itself, allowing it to be GC'd later. Currently model objects store all of that intermediate stuff.
  • Combine the storage- and factorization-specific types (DensePredChol et al.) into a single LinearPredictor type with parameters that determine eltype, storage, and factorization. I think this would potentially allow more flexibility in terms of extensions, as no new type is needed, just some method definitions.
  • Remove the response-specific types, LmResp and GlmResp, in favor of storing the relevant bits in the model objects directly. I didn't get this far on my branch. These types always seemed strange to me since weights, for example, aren't really part of the model's response as such. (At least not in my mind.)
  • Define accessor functions via StatsAPI and use those internally rather than always referencing pp and rr fields. In at least one instance I encountered in the past, the use of .rr internally was incorrect and caused an error, which would have made more sense were it a method error from response.

I figured I'd open this issue in case anybody likes any of these ideas enough to open a PR, since it's quite unlikely that I'll end up getting my local branch to a PR-worthy state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant