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

Automatic inverse link #56

Merged
merged 15 commits into from
Aug 21, 2023
Merged

Automatic inverse link #56

merged 15 commits into from
Aug 21, 2023

Conversation

palday
Copy link
Member

@palday palday commented Aug 19, 2023

  • adds a way to create extensions for automatic inverse link determination
  • defines such extensions for GLM and MixedModels
  • this also allows using analytic deritvatives instead of AD for the difference method

@palday palday marked this pull request as ready for review August 19, 2023 22:27
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #56 (e838625) into main (1cd621d) will decrease coverage by 0.58%.
The diff coverage is 95.65%.

@@             Coverage Diff             @@
##              main      #56      +/-   ##
===========================================
- Coverage   100.00%   99.42%   -0.58%     
===========================================
  Files            4        6       +2     
  Lines          154      174      +20     
===========================================
+ Hits           154      173      +19     
- Misses           0        1       +1     
Files Changed Coverage Δ
src/Effects.jl 100.00% <ø> (ø)
src/regressionmodel.jl 97.77% <94.44%> (-2.23%) ⬇️
ext/EffectsGLMExt.jl 100.00% <100.00%> (ø)
ext/EffectsMixedModelsExt.jl 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

src/regressionmodel.jl Outdated Show resolved Hide resolved
Copy link
Member

@ararslan ararslan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be simplified considerably by defining more generics directly in Effects that will then be extended when loading GLM or any package that depends on it.

ext/EffectsGLMExt.jl Outdated Show resolved Hide resolved
Co-authored-by: Alex Arslan <[email protected]>
ext/EffectsMixedModelsExt.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
test/delta_method.jl Outdated Show resolved Hide resolved
palday and others added 3 commits August 21, 2023 18:38
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

[extensions]
EffectsGLMExt = "GLM"
EffectsMixedModelsExt = ["GLM", "MixedModels"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC this will only be loaded if both MixedModels and GLM are loaded. I don't think that restriction is necessary since MixedModels depends on GLM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤷 I wanted to be explicit

ext/EffectsGLMExt.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
Comment on lines +73 to +75
@test all(pred.prediction .≈ eff.vol)
@test all(isapprox.(pred.lower, eff.lower; atol=0.001))
@test all(isapprox.(pred.upper, eff.upper; atol=0.001))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@test all(pred.prediction . eff.vol)
@test all(isapprox.(pred.lower, eff.lower; atol=0.001))
@test all(isapprox.(pred.upper, eff.upper; atol=0.001))
@test pred.prediction eff.vol
@test pred.lower eff.lower atol=0.001
@test pred.upper eff.upper atol=0.001

Any reason not to test the approximate equality of the arrays directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because we want element-wise approximate equality, which is a stronger condition (the approximate equality of vectors is based on the norm of the difference vector)

Comment on lines +83 to +85
@test all(eff_trans.vol .≈ eff.vol)
@test all(isapprox.(eff_trans.lower, eff.lower; atol=0.001))
@test all(isapprox.(eff_trans.upper, eff.upper; atol=0.001))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@test all(eff_trans.vol . eff.vol)
@test all(isapprox.(eff_trans.lower, eff.lower; atol=0.001))
@test all(isapprox.(eff_trans.upper, eff.upper; atol=0.001))
@test eff_trans.vol eff.vol
@test eff_trans.lower eff.lower atol=0.001
@test eff_trans.upper eff.upper atol=0.001

Same question as above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer as above

palday and others added 2 commits August 21, 2023 16:19
Co-authored-by: Alex Arslan <[email protected]>
Co-authored-by: Alex Arslan <[email protected]>
@palday palday merged commit 1ca3e9d into main Aug 21, 2023
8 of 10 checks passed
@palday palday deleted the pa/glmext branch August 21, 2023 21:39
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

Successfully merging this pull request may close these issues.

3 participants