search

FixedEffects / Vcov.jl

Variance Covariance Matrices for developers
Other
9 stars 4 forks source link

ClusterCovariance does not have a type parameter #1

Closed junyuan-chen closed 3 years ago

junyuan-chen commented 3 years ago

Is there a good reason why the type parameter of ClusterCovariance is removed after commit https://github.com/matthieugomez/Vcov.jl/commit/008bfe7fc70d3a3cbddf431feb4f2c53f3a29b15 ?

I thought having a type parameter is useful here as functions such as Vcov.completecases and nclusters are assuming some specific types of the field clusters. In the former case, clusters is a tuple of Symbols; while in the latter case, clusters is a named tuple of GroupedArrays. With a type parameter, such assumptions can be explicitly incorporated into the method definitions.

(I could make a pull request on that if needed.)

matthieugomez commented 3 years ago

I think I did this to avoid excessive specialization (say on the name of the arrays). I guess we could revert to a type parameter and add @nospecialize in the arguments of the function.

On Sun, Feb 14, 2021 at 10:09 AM Norman notifications@github.com wrote:

Is there a good reason why the type parameter of ClusterCovariance is removed after commit 008bfe7 https://github.com/matthieugomez/Vcov.jl/commit/008bfe7fc70d3a3cbddf431feb4f2c53f3a29b15 ?

I thought having a type parameter is useful here as functions such as Vcov.completecases and nclusters are assuming some specific types of the field clusters. In the former case, clusters is a tuple of Symbols; while in the latter case, clusters is a named tuple of GroupedArray. With a type parameter, such assumptions can be explicitly incorporated into the method definitions.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/matthieugomez/Vcov.jl/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPPPXJHZJC5QLNN2REHMYDS7AGVFANCNFSM4XTOVBPQ .

junyuan-chen commented 3 years ago

Ok, I see. Then, maybe instead of only having one field that is sometimes names and sometimes names paired with arrays, we could have two fields with the first one always being a tuple of names and the second one being a tuple of GroupedArrays.

I think I did this to avoid excessive specialization (say on the name of the arrays). I guess we could revert to a type parameter and add @nospecialize in the arguments of the function. … On Sun, Feb 14, 2021 at 10:09 AM Norman @.***> wrote: Is there a good reason why the type parameter of ClusterCovariance is removed after commit 008bfe7 <008bfe7> ? I thought having a type parameter is useful here as functions such as Vcov.completecases and nclusters are assuming some specific types of the field clusters. In the former case, clusters is a tuple of Symbols; while in the latter case, clusters is a named tuple of GroupedArray. With a type parameter, such assumptions can be explicitly incorporated into the method definitions. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPPPXJHZJC5QLNN2REHMYDS7AGVFANCNFSM4XTOVBPQ .