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

Adds values_as_in_model #588

Merged
merged 13 commits into from
Apr 19, 2024
Merged

Adds values_as_in_model #588

merged 13 commits into from
Apr 19, 2024

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented Apr 18, 2024

Related: TuringLang/Turing.jl#2195

I also added a docstring which I believe explains the issue fairly well.

EDIT: To actually address TuringLang/Turing.jl#2195 we need some way knowing whether a model has static constraints or not. I made a commit adding a isstatic field to Model to test it out, and it works nicely. But we probably want something slightly more general, i.e. a "trait system" with traits such as:

  • has static varnames, i.e. the varnames do not change between realizations
  • has static transformations, i.e. transformations are not dependent on the values of the model

And maybe more.

Without something like this I don't quite see how we can solve these issues 😕

EDIT 2: Remove the static-trait-thingy.

corresponding to a varname from a varinfo as seen in the model
`extract_realizations` so we sample variables not present in the varinfo
@coveralls
Copy link

coveralls commented Apr 18, 2024

Pull Request Test Coverage Report for Build 8758055720

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 28 of 40 (70.0%) changed or added relevant lines in 1 file are covered.
  • 25 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-1.8%) to 80.055%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/values_as_in_model.jl 28 40 70.0%
Files with Coverage Reduction New Missed Lines %
src/model.jl 1 89.22%
src/varinfo.jl 6 85.98%
src/simple_varinfo.jl 6 82.61%
src/threadsafe.jl 12 51.75%
Totals Coverage Status
Change from base Build 8745687397: -1.8%
Covered Lines: 2605
Relevant Lines: 3254

💛 - Coveralls

@sunxd3
Copy link
Member

sunxd3 commented Apr 19, 2024

This makes great sense to me. Some concerns:

  • RealizationExtractorContext feels like a very specific name for a very specific use case. Are there other use cases where we can generalize this context a bit? For instance, you mentioned at Issues with constrained parameters depending on each other Turing.jl#2195 (comment) Gibbs might make use of this
  • The name extract_realizations might not be obvious what the function does (for instance what is a "realization" and where does it "extract" from), I don't have a better name suggestion, though
  • do we want to make is_static a bit more specific like has_static_support or something? Given that static can meaning other things, for instance having consistent parameter dimension?

@torfjelde
Copy link
Member Author

The name extract_realizations might not be obvious what the function does (for instance what is a "realization" and where does it "extract" from), I don't have a better name suggestion, though

"realization" has a very clear meaning in the context of random variables, but I agree overall that extract_realizations is maybe not the best name 👍

Happy to take alternative suggestions! I do think the docstring clarifies it a bit, but still.

do we want to make is_static a bit more specific like has_static_support or something? Given that static can meaning other things, for instance having consistent parameter dimension?

Big in favour of this too 👍

@torfjelde
Copy link
Member Author

RealizationExtractorContext feels like a very specific name for a very specific use case. Are there other use cases where we can generalize this context a bit? For instance, you mentioned at TuringLang/Turing.jl#2195 (comment) Gibbs might make use of this

I was thinking about this a bit earlier, and tbh it might be worth moving this "has_static_support" check into invlink itself to force re-computation unless we're certain it's not needed.

But what sort of generalizations do you have in mind? The gibbs use-case would benefit from exactly the same if statement I mentioned in that issue

src/model.jl Outdated Show resolved Hide resolved
src/model.jl Outdated Show resolved Hide resolved
torfjelde and others added 2 commits April 19, 2024 16:43
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@torfjelde
Copy link
Member Author

After chat with @yebai, we've decided to remove the has_static_support for now.

@torfjelde
Copy link
Member Author

Am in dire need of a better name though!

more descriptive (and similarly for the corresponding context)
@torfjelde torfjelde changed the title Adds extract_realizations Adds values_as_in_model Apr 19, 2024
@torfjelde torfjelde merged commit af64356 into master Apr 19, 2024
3 of 10 checks passed
@torfjelde torfjelde deleted the torfjelde/extract-realizations branch April 19, 2024 18:32
@torfjelde
Copy link
Member Author

Shieeet I just accidentally merged the wrong way 🤦

@sunxd3
Copy link
Member

sunxd3 commented Apr 19, 2024

That's fine

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