Skip to content

[Merged by Bors] - fix partial missing bug #191

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
Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - fix partial missing bug #191

wants to merge 6 commits into from

Conversation

@mohdibntarek
Copy link
Contributor

@mohdibntarek mohdibntarek commented Nov 21, 2020

This PR fixes TuringLang/Turing.jl#1464.

devmotion
devmotion requested changes Nov 21, 2020
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Wasn't the suggestion to not always deepcopy? Please also add a test for this (without using Turing preferably) and bump the version.

@mohdibntarek
Copy link
Contributor Author

mohdibntarek commented Nov 21, 2020

I added a test but had to move some get_matching_type methods from Turing which were type pirating DPPL anyways. I can increment the version separately.

devmotion
devmotion requested changes Nov 21, 2020
View reviewed changes
@devmotion
Copy link
Member

devmotion commented Nov 21, 2020

I can increment the version separately.

ColPrac (and the protection of the master branch) require the version to be incremented in PRs.

@mohdibntarek mohdibntarek mentioned this pull request Nov 21, 2020
Merged
torfjelde
torfjelde requested changes Nov 21, 2020
View reviewed changes
Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

Looks good! Other than the version-bump I'd recommend adding in some tests for the behavior that made us discover this bug, to ensure that it doesn't happen in the future.

@torfjelde
Copy link
Member

torfjelde commented Dec 11, 2020

@devmotion @mohamed82008 Are we waiting for TuringLang/Turing.jl#1465 before merging this?

@devmotion
Copy link
Member

devmotion commented Dec 11, 2020

We mainly wait for the comments to be addressed and the PR to be rebased or updated. However, IMO it is unfortunate that this requires changes in Turing - the fix does not need it so it would be nice to leave this out of the PR.

@devmotion
Copy link
Member

devmotion commented Dec 11, 2020

The main annoyance of the Turing changes is that they require us to drop support for all current versions of DynamicPPL.

@torfjelde
Copy link
Member

torfjelde commented Dec 12, 2020 •
edited
Loading

We mainly wait for the comments to be addressed and the PR to be rebased or updated.

But given @mohamed82008 's absence, can we/I just go ahead and make the changes to get it merged?

However, IMO it is unfortunate that this requires changes in Turing - the fix does not need it so it would be nice to leave this out of the PR.

Hmm, how can we do this without removing the changes in Turing? Seems like there's no way around it since we want to add default behavior of a particular method which Turing currently has a wrong implementation for.

The main annoyance of the Turing changes is that they require us to drop support for all current versions of DynamicPPL.

Fair point, but there's not going to be a way to circumvent this, right? Or are you suggesting that we instead only make the changes in Turing and leave DPPL without a proper implementation?

@devmotion
Copy link
Member

devmotion commented Dec 12, 2020

Yes, I think we should go ahead, apply the suggested changes, bump the version (and the bounds in Turing) and release everything. These methods actually belong in DynamicPPL and not in Turing, so at some point we would have to move them anyway.

@devmotion devmotion force-pushed the mt/fix_partial_missing branch from 61f9a55 to bf3e7d6 Compare December 12, 2020 13:02
devmotion
devmotion approved these changes Dec 12, 2020
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

LGTM.

@devmotion devmotion requested a review from torfjelde December 12, 2020 13:09
torfjelde
torfjelde approved these changes Dec 12, 2020
View reviewed changes
Copy link
Member

@torfjelde torfjelde left a comment

Choose a reason for hiding this comment

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

LGTM 2.

@devmotion
Copy link
Member

devmotion commented Dec 12, 2020

bors r+

bors bot pushed a commit that referenced this pull request Dec 12, 2020
@mohdibntarek @devmotion
fix partial missing bug (#191)
f9adfeb 
This PR fixes TuringLang/Turing.jl#1464.

Co-authored-by: David Widmann <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 12, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix partial missing bug [Merged by Bors] - fix partial missing bug Dec 12, 2020
@bors bors bot closed this Dec 12, 2020
@bors bors bot deleted the mt/fix_partial_missing branch December 12, 2020 19:09
@torfjelde torfjelde mentioned this pull request Dec 16, 2020
Closed
@penelopeysm penelopeysm mentioned this pull request Aug 7, 2025
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devmotion devmotion devmotion approved these changes

@torfjelde torfjelde torfjelde approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Error when running example on missing values

4 participants

@mohdibntarek @devmotion @torfjelde