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.

Sign up for GitHub

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

Jump to bottom

Remove link functions and refactor tests #86

Merged

sunxd3 merged 14 commits into master from sunxd/remove-link-function

Sep 8, 2023

Member

sunxd3 commented Sep 7, 2023 •

edited

Loading

Link function field is removed from NodeInfo
- This only affects link function in stochastic assignments, not logical assignments. Because in the latter, we already transformed the expression by calling inverse function of the link function on the RHS
- Nimble supports this syntax while the original BUGS doesn't, so this is not really a loss of feature
- Also with Bijectors.jl, the benefits of supporting link functions in stochastic assignments is diminishing. In the future, we can add support for specifying the bijector to use, which would be a solid improvement over the link function syntax in my opinion
The logp computation is refactored a little bit
- The way we handled link function before is to first push dist through a bijector, and then pull it back and accumulate logabsdetjac. Now this is removed
- Also a bugs fix: with DynamicPPL.reconstruct, matrix values are handled correctly
- Trans-dimensional bijectors are not handled in this PR, but will be fixed very soon
Some tests are refactored
- Previously, we rely on overloading behavior of DynamicPPL, now we just use link to transform the varinfo; use LogDensityFunction when possible

sunxd3 added 4 commits

September 7, 2023 13:41


          Remove link function from graph nodes

46a0039


          Refactor some tests

f20d378


          Update tests, use link or LogDensityFunction

783c063


          Move bijector definitions to utils.jl

bedff95

github-actions bot reviewed

View reviewed changes

src/graphs.jl Outdated

Comment on lines 74 to 76

                       check_and_add_vertex!(
-                          g, l_vn, NodeInfo(l, vars, link_functions, node_functions, node_args)
+                          g, l_vn, NodeInfo(l, vars, node_functions, node_args)
                       )

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                    check_and_add_vertex!(
          
                        g, l_vn, NodeInfo(l, vars, link_functions, node_functions, node_args)
          
                        g, l_vn, NodeInfo(l, vars, node_functions, node_args)
          
                    )
          
                    check_and_add_vertex!(g, l_vn, NodeInfo(l, vars, node_functions, node_args))

src/graphs.jl Outdated

Comment on lines 82 to 84

                           check_and_add_vertex!(
-                              g, r_vn, NodeInfo(r, vars, link_functions, node_functions, node_args)
+                              g, r_vn, NodeInfo(r, vars, node_functions, node_args)
                           )

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                        check_and_add_vertex!(
          
                            g, r_vn, NodeInfo(r, vars, link_functions, node_functions, node_args)
          
                            g, r_vn, NodeInfo(r, vars, node_functions, node_args)
          
                        )
          
                        check_and_add_vertex!(g, r_vn, NodeInfo(r, vars, node_functions, node_args))

test/logp_tests/binomial.jl Outdated

Comment on lines 15 to 17

+                  JuliaBUGS.evaluate!!(
+                      DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
+                  ).logp

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                JuliaBUGS.evaluate!!(
          
                    DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
          
                ).logp
          
                JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, false), DefaultContext()).logp

test/logp_tests/blockers.jl Outdated

Comment on lines 38 to 40

+                  JuliaBUGS.evaluate!!(
+                      DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
+                  ).logp

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                JuliaBUGS.evaluate!!(
          
                    DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
          
                ).logp
          
                JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, false), DefaultContext()).logp

test/logp_tests/bones.jl Outdated

Comment on lines 40 to 42

+                  JuliaBUGS.evaluate!!(
+                      DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
+                  ).logp

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                JuliaBUGS.evaluate!!(
          
                    DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
          
                ).logp
          
                JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, false), DefaultContext()).logp

test/logp_tests/dogs.jl Outdated

Comment on lines 43 to 45

    
                  JuliaBUGS.evaluate!!(

                      DynamicPPL.settrans!!(bugs_model, false), DefaultContext()

                  ).logp

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                JuliaBUGS.evaluate!!(
          
                    DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
          
                ).logp
          
                JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, false), DefaultContext()).logp

test/logp_tests/gamma.jl Outdated

Comment on lines 16 to 18

+                  JuliaBUGS.evaluate!!(
+                      DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
+                  ).logp

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                JuliaBUGS.evaluate!!(
          
                    DynamicPPL.settrans!!(bugs_model, false), DefaultContext()
          
                ).logp
          
                JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, false), DefaultContext()).logp

test/logp_tests/rats.jl Outdated

Comment on lines 75 to 76

		)[2].logp
		bugs_logp = JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, true), DefaultContext()).logp

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                )[2].logp 
          
            bugs_logp = JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, true), DefaultContext()).logp
          
                )[2].logp
          
            bugs_logp =
          
                JuliaBUGS.evaluate!!(DynamicPPL.settrans!!(bugs_model, true), DefaultContext()).logp

sunxd3 added 5 commits

September 7, 2023 14:33


          Formatting

1f8ad8d


          Raise version to 0.2.0, add some dep to testenv

7eeb26d


          Add test for LogDensityContext for markov model

ec41bd3


          Remove :lkj from the test - not implemented yet

6e7c1f0


          Bug fix

6b82233

codecov bot commented Sep 7, 2023 •

edited

Loading

Codecov Report

Patch coverage: 64.47% and project coverage change: +2.54% 🎉

Comparison is base (8bcf530) 69.20% compared to head (e974aba) 71.75%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
+ Coverage   69.20%   71.75%   +2.54%     
==========================================
  Files          17       16       -1     
  Lines        1640     1625      -15     
==========================================
+ Hits         1135     1166      +31     
+ Misses        505      459      -46

Files Changed	Coverage Δ
src/utils.jl	`71.92% <25.00%> (-18.32%)`	⬇️
src/model.jl	`78.84% <70.21%> (+14.66%)`	⬆️
src/compiler_pass.jl	`91.40% <80.00%> (-0.24%)`	⬇️
src/JuliaBUGS.jl	`94.87% <100.00%> (ø)`
src/graphs.jl	`75.38% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

sunxd3 requested a review from yebai

September 7, 2023 14:32


          Remove link_function from NodeFunctions pass

59d0915

github-actions bot reviewed

View reviewed changes

src/compiler_pass.jl Outdated

Comment on lines 819 to 822

                   pass.array_sizes,
                   pass.array_bitmap,
-                  pass.link_functions,
                   pass.node_args,
                   pass.node_functions,

Contributor

github-actions bot Sep 7, 2023

[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

      
                pass.array_sizes,
          
                pass.array_bitmap,
          
                pass.link_functions,
          
                pass.node_args,
          
                pass.node_functions,
          
                pass.array_sizes, pass.array_bitmap, pass.node_args, pass.node_functions,


          Formatting

b06dede

yebai reviewed

View reviewed changes

Member

yebai left a comment

Nice work @sunxd3 -- I left some minor comments below.

src/compiler_pass.jl Outdated Show resolved Hide resolved

src/utils.jl Outdated

+              Bijectors.transform(::Inverse{LogisticBijector}, x::Real) = logit(x)
+              Bijectors.logabsdet(::LogisticBijector, x::Real) = log(logistic(x)) + log(1 - logistic(x))
+              struct CExpExp <: Bijectors.Bijector end

Member

yebai Sep 7, 2023

Consider adding suffix Bijector to all bijectors here for consistency and clarity.

Suggested change

      
            struct CExpExp <: Bijectors.Bijector end
          
            struct CExpExpBijector <: Bijectors.Bijector end

src/utils.jl Outdated

+              Bijectors.transform(::Inverse{ExpBijector}, x::Real) = log(x)
+              Bijectors.logabsdet(::ExpBijector, x::Real) = x
+              struct Phi <: Bijectors.Bijector end

Member

yebai Sep 7, 2023

Suggested change

      
            struct Phi <: Bijectors.Bijector end
          
            struct PhiBijector <: Bijectors.Bijector end

Member

yebai commented Sep 7, 2023

In the future, we can add support for specifying the bijector to use, which would be a solid improvement over the link function syntax in my opinion

I am not sure what this means. Can you elaborate?


          Apply suggestions from @yebai

7d0fd8a

Member Author

sunxd3 commented Sep 7, 2023

Sorry that was confusing -- I just mean we can allow user to specify what bijector they want to use on certain variables. Not sure if people actually want this. The priority is pretty low by my measure.

Member

yebai commented Sep 7, 2023

Sorry that was confusing -- I just mean we can allow user to specify what bijector they want to use on certain variables. Not sure if people actually want this. The priority is pretty low by my measure.

I see, for clarity, link functions inside the model specification are different from bijective transforms we use during inference. The former is part of the model, and cannot be replaced with a inference time bijector.

Member

yebai commented Sep 7, 2023

Nimble supports this syntax while the original BUGS doesn't, so this is not really a loss of feature

Do you have a reference on this?


          Fix error

e974aba

Member Author

sunxd3 commented Sep 8, 2023

Do you have a reference on this?

Nimble Doc
OpenBUGS only mention link function in logical nodes; I also verified with MultiBUGS that link function in stochastic assignment produce compilation errors.


          Merge remote-tracking branch 'origin/master' into sunxd/remove-link-f…

d91707b

…unction

sunxd3 merged commit 05b4e03 into master

11 checks passed

sunxd3 deleted the sunxd/remove-link-function branch

September 8, 2023 06:35

sunxd3 mentioned this pull request

Refactor graph and model part of the code #80

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet