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

PriorExtractorContext #496

Merged
merged 29 commits into from
Jul 13, 2023
Merged

PriorExtractorContext #496

merged 29 commits into from
Jul 13, 2023

Conversation

JaimeRZP
Copy link
Member

Implements the tool to extract the priors from a model proposed by @torfjelde in #2009. Previously in #2031.

src/context_implementations.jl Outdated Show resolved Hide resolved
src/context_implementations.jl Outdated Show resolved Hide resolved
src/context_implementations.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
JaimeRZP and others added 2 commits July 11, 2023 14:13
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@coveralls
Copy link

coveralls commented Jul 11, 2023

Pull Request Test Coverage Report for Build 5535103664

  • 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 76.736%

Totals Coverage Status
Change from base Build 5358326778: 0.3%
Covered Lines: 1956
Relevant Lines: 2549

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.32 🎉

Comparison is base (e6dd4ef) 76.40% compared to head (14891c8) 76.73%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #496      +/-   ##
==========================================
+ Coverage   76.40%   76.73%   +0.32%     
==========================================
  Files          21       22       +1     
  Lines        2522     2549      +27     
==========================================
+ Hits         1927     1956      +29     
+ Misses        595      593       -2     
Impacted Files Coverage Δ
src/DynamicPPL.jl 100.00% <ø> (ø)
src/extract_priors.jl 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@JaimeRZP
Copy link
Member Author

Hi @torfjelde & @devmotion!

Do you have any suggestion on how to test the new context?

Many thanks,
Jaime

src/contexts.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

Do you have any suggestion on how to test the new context?

I guess the natural thing would be to test it with different models (the set of test models?) and check that the extracted priors are correct.

JaimeRZP and others added 2 commits July 12, 2023 14:32
Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
src/contexts.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
test/model.jl Outdated Show resolved Hide resolved
JaimeRZP and others added 4 commits July 12, 2023 15:22
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>
test/model.jl Outdated Show resolved Hide resolved
JaimeRZP and others added 3 commits July 12, 2023 16:01
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
test/model.jl Outdated Show resolved Hide resolved
src/context_implementations.jl Outdated Show resolved Hide resolved
src/context_implementations.jl Outdated Show resolved Hide resolved
src/context_implementations.jl Outdated Show resolved Hide resolved
src/contexts.jl Outdated Show resolved Hide resolved
src/DynamicPPL.jl Outdated Show resolved Hide resolved
test/model.jl Outdated Show resolved Hide resolved
torfjelde and others added 4 commits July 12, 2023 16:32
@@ -102,6 +102,12 @@ For a chain of samples, one can compute the pointwise log-likelihoods of each ob
pointwise_loglikelihoods
```

Sometimes it can be useful to extract the priors of a model. This is the possible using [`extract_priors`](@ref).
Copy link
Member

Choose a reason for hiding this comment

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

If I understood this PR corrently, we are extracting the conditional priors of model parameters, i.e., priors conditioning on values of their parent nodes.

Suggested change
Sometimes it can be useful to extract the priors of a model. This is the possible using [`extract_priors`](@ref).
Sometimes it can be helpful to to extract the (conditional) priors of a model. This is possible using [`extract_priors`](@ref).

Copy link
Member

Choose a reason for hiding this comment

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

If I understood this PR corrently, we are extracting the conditional priors of model parameters, i.e., priors conditioning on values of their parent nodes.

Is this distinction necessary? For example, what is the difference between "the prior" and "the conditional prior" in x ~ Normal(); y ~ Normal(x, 1)? I don't think I completely follow.

Copy link
Member

@yebai yebai Jul 13, 2023

Choose a reason for hiding this comment

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

My understanding is that we will not preserve the dependency structure of variables. In the example above, we will obtain prior_x=Normal(), prior_y=Normal(value_x, 1), where value_x = rand(Normal()). So the dependency information of y on x is lost.

@torfjelde
Copy link
Member

This issue with the docs is very strange. Basically, the doctests pass just fine when run in runtests.jl, but when they're run as part of the documentation process, it fails because now the qualifier is added to the results 🤷

Have you come across this before @devmotion ?

docs/make.jl Outdated Show resolved Hide resolved
@torfjelde
Copy link
Member

torfjelde commented Jul 12, 2023

Okay so "fixed" the issue by adding also Distributions available in the namespace of the makedocs call (similar to how it is in test/runtests.jl). The fact that this happens is still weird to me though.

EDIT: "weird" in the sense that I haven't before realized whether the qualifier is included or not, despite where the print statement occurs, is based on whether that namespace has been included "globally".

test/runtests.jl Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@yebai yebai enabled auto-merge July 13, 2023 19:13
@yebai yebai added this pull request to the merge queue Jul 13, 2023
Merged via the queue into master with commit e8172f0 Jul 13, 2023
13 of 14 checks passed
@yebai yebai deleted the PriorContext branch July 13, 2023 19:32
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.

None yet

5 participants