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

Re-export certain functionality from DynamicPPL #2217

Merged
merged 6 commits into from
May 18, 2024
Merged

Conversation

torfjelde
Copy link
Member

Given that Turing.jl has become a "public-facing" shell package, I think it's worth exporting some functionality commonly used with models from DynamicPPL.jl, so the end-user doesn't have to see DynamicPPL to be able to use these.

Amongst these I think the following should be included:

  • condition
  • decondition
  • fix
  • unfix
  • OrderedDict (the only reliable way to call many methods, e.g. rand)
  • conditioned / observations (though probably just one of these?)

Any objections? @yebai @devmotion

@yebai
Copy link
Member

yebai commented May 8, 2024

Looks good to me -- let's export conditioned instead of observations for consistency with condition/decondition APIs

@coveralls
Copy link

coveralls commented May 8, 2024

Pull Request Test Coverage Report for Build 9141352740

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

Totals Coverage Status
Change from base Build 9140976557: 0.0%
Covered Lines: 0
Relevant Lines: 1529

💛 - Coveralls

src/Turing.jl Outdated Show resolved Hide resolved
@torfjelde
Copy link
Member Author

Okay, these tests failing are now becoming somewhat worrying (previously everything passed except on one run, but now it's also failing on other systems)

@yebai yebai merged commit 2478d6a into master May 18, 2024
11 checks passed
@yebai yebai deleted the torfjelde/exports branch May 18, 2024 18:51
@torfjelde
Copy link
Member Author

Why was this merged?

@torfjelde
Copy link
Member Author

@yebai

@torfjelde
Copy link
Member Author

torfjelde commented May 18, 2024

I specifically said "please don't do this" just a few days ago.

@torfjelde
Copy link
Member Author

I see tests are at least passing after the merge, but still, please don't do this. Ping the creator of the PR to check first.

@yebai
Copy link
Member

yebai commented May 18, 2024

Let's be flexible about merging. Core members should be allowed to merge PRs considered "ready to go" -- it's debatable what can be treated as "ready to go", but this PR probably fits into that group safely I think.

@torfjelde
Copy link
Member Author

torfjelde commented May 18, 2024

But what arguments exist for merging vs. pinging the creator of the PR and asking them to merge @yebai ? I can't see any in favour of doing this.

In contrast, I can easily come up with arguments for not doing this, e.g. we've had examples before where the PR creator realizes the test cases aren't sufficient, starts working on it, tries to push, only to realize the branch has already been merged and released with bugs the PR creator was in the process of fixing.

@torfjelde
Copy link
Member Author

torfjelde commented May 18, 2024

But regardless of whether we want to make "don't merge someone else's PR without checking with them first" a team-wide rule or not, I ask that you at least don't merge PRs that I have a created but instead ping me and ask if it's ready to merge.
If I'm being very unresponsive, then sure, feel free to merge, but otherwise let me have the final say in whether a PR I have made is ready or not.

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.

4 participants