FixedEffects / FixedEffectModels.jl

Fast Estimation of Linear Models with IV and High Dimensional Categorical Variables
Other
225 stars 46 forks source link

Fix Aqua unbound arg error #247

Closed nilshg closed 10 months ago

nilshg commented 10 months ago

The current definition of Combination introduces an implicit zero-arg method which means the type parameter in Combination(A::Union{AbstractMatrix{T}, AbstractVector{T}}) where T becomes unbounded. This leads to additional work for the compiler which can lead to latency issues.

Explicitly adding a zero-arg method Combination() which errors solves this - as far as I can see there's no sensible way in which a zero-arg version could be used, it errors currently anyway and throwing an explicity error is a UX improvement (if marginal as this error would probably encounter somewhere in the stack without the user calling Combination() themselves).

Happily this is the only Aqua test failure for FixedEffectModels, so we could think about adding Aqua to CI and displaying an Aqua badge, which more and more users seem to look for as a basic quality indicator.

codecov[bot] commented 10 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4c5e187) 96.35% compared to head (27bd9e8) 96.21%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #247 +/- ## ========================================== - Coverage 96.35% 96.21% -0.15% ========================================== Files 8 8 Lines 659 660 +1 ========================================== Hits 635 635 - Misses 24 25 +1 ``` | [Files](https://app.codecov.io/gh/FixedEffects/FixedEffectModels.jl/pull/247?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/utils/basecol.jl](https://app.codecov.io/gh/FixedEffects/FixedEffectModels.jl/pull/247?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3V0aWxzL2Jhc2Vjb2wuamw=) | `98.07% <0.00%> (-1.93%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

matthieugomez commented 10 months ago

Thanks!