Make IdentityOperator default for KLMinRepGradDescent
#201
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e_default_operator_advi
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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>
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>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
AdvancedVI.jl documentation for PR #201 is available at: |
…ngLang/AdvancedVI.jl into change_default_operator_advi
There was a problem hiding this comment.
Benchmark Results
| Benchmark suite | Current: b5f30b6 | Previous: 89792f5 | Ratio |
|---|---|---|---|
normal/RepGradELBO + STL/meanfield/Zygote |
3854411925.5 ns |
3885127850.5 ns |
0.99 |
normal/RepGradELBO + STL/meanfield/ReverseDiff |
1132239771 ns |
1134192546 ns |
1.00 |
normal/RepGradELBO + STL/meanfield/Mooncake |
1229203243 ns |
1189440216 ns |
1.03 |
normal/RepGradELBO + STL/fullrank/Zygote |
3849984126 ns |
3875571674 ns |
0.99 |
normal/RepGradELBO + STL/fullrank/ReverseDiff |
1625327857 ns |
1630205282.5 ns |
1.00 |
normal/RepGradELBO + STL/fullrank/Mooncake |
1253912550 ns |
1251609883 ns |
1.00 |
normal/RepGradELBO/meanfield/Zygote |
2688901547 ns |
2776185010 ns |
0.97 |
normal/RepGradELBO/meanfield/ReverseDiff |
780829400 ns |
796550877 ns |
0.98 |
normal/RepGradELBO/meanfield/Mooncake |
1090922281 ns |
1077307075 ns |
1.01 |
normal/RepGradELBO/fullrank/Zygote |
2742295948.5 ns |
2781660852.5 ns |
0.99 |
normal/RepGradELBO/fullrank/ReverseDiff |
958847335 ns |
947239522.5 ns |
1.01 |
normal/RepGradELBO/fullrank/Mooncake |
1136824194 ns |
1117639731 ns |
1.02 |
normal + bijector/RepGradELBO + STL/meanfield/Zygote |
5521801742 ns |
5507943890 ns |
1.00 |
normal + bijector/RepGradELBO + STL/meanfield/ReverseDiff |
2412856875 ns |
2400012438 ns |
1.01 |
normal + bijector/RepGradELBO + STL/meanfield/Mooncake |
4028904376.5 ns |
4067417017.5 ns |
0.99 |
normal + bijector/RepGradELBO + STL/fullrank/Zygote |
5583900806 ns |
5518202036 ns |
1.01 |
normal + bijector/RepGradELBO + STL/fullrank/ReverseDiff |
3040109403 ns |
3002283069 ns |
1.01 |
normal + bijector/RepGradELBO + STL/fullrank/Mooncake |
4119691765.5 ns |
4121790814.5 ns |
1.00 |
normal + bijector/RepGradELBO/meanfield/Zygote |
4370908715 ns |
4221420082.5 ns |
1.04 |
normal + bijector/RepGradELBO/meanfield/ReverseDiff |
2030847373 ns |
2000407104 ns |
1.02 |
normal + bijector/RepGradELBO/meanfield/Mooncake |
3864032093.5 ns |
3872951671.5 ns |
1.00 |
normal + bijector/RepGradELBO/fullrank/Zygote |
4405243285.5 ns |
4329628829 ns |
1.02 |
normal + bijector/RepGradELBO/fullrank/ReverseDiff |
2287718739 ns |
2255868329 ns |
1.01 |
normal + bijector/RepGradELBO/fullrank/Mooncake |
3930085455.5 ns |
4022021340.5 ns |
0.98 |
This comment was automatically generated by workflow using github-action-benchmark.
|
Gentle ping for anybody around @zuhengxu @sunxd3 @penelopeysm |
|
This looks good to me if we do want to use I'd like to hear about @penelopeysm's and @sunxd3's opinion. |
|
@penelopeysm @sunxd3 Gentle ping |
|
@penelopeysm @sunxd3 Gentle ping |
|
@yebai @sunxd3 @penelopeysm Gentle ping. Sorry for the hassle you all! |
penelopeysm
reviewed
Sep 14, 2025
penelopeysm
approved these changes
Sep 14, 2025
|
@penelopeysm Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, for
KLMinReGradDescent, the defaultoperatorkeyword argument wasClipScale(). Now,ClipScaleassumes the variational family is<:MvLocationScale. So if a different variational family is used, it would return an error. This will probably be confusing to the users ( although I expect location-scales to be used most of the time, so it was a decision optimized for this use case). On the other hand, when using location-scales, it is essential to useClipScale, which has to be communicated in some way.This PR fixes the issues above as follows:
IdentityOperatoris now the default value foroperator. However, whenever a<:MvLocationScalefamily is used in combination withIdentityOperator, a warning will be displayed explaining that this is not a good idea while instructing to useClipScale.