Skip to content

Conversation

@devmotion
Copy link
Member

Fix #1756 by renaming Turing.Core to Turing.Essential (maybe there's a better name? Shouldn't choose Base or Main though 😛).

@torfjelde
Copy link
Member

torfjelde commented Jan 14, 2022

Ah Essential is nicer than what I had in mind (e.g. TuringCore) 👍

But should we maybe bump the minor version? I know Core is technically not exported, but I know a lot of people's code will break, e.g. I have snippets benchmarking AD backends that will now break.

EDIT: Though the reason I went with TuringCore is because figured that we'd never run into namespace-clashes with any other package using that 😅 But I don't like it..

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! As long as tests pass 👍

Only thing I'd like to change is bumping minor version instead of patch version (as explained in the comment above).

@devmotion
Copy link
Member Author

Hmm yeah, probably safer. I wonder though if we could maybe even deprecate it with @deprecate_binding only in the top level Turing module, I hope that this would still fix the issue AND we could distribute this fix to all people with Turing.Core snippets (otherwise the have to pin their Julia versions manually to an older release). But I'm not sure if it's worth it (and of course I have to check first if the issue is still fixed).

@torfjelde
Copy link
Member

Uuuh I did not know about @deprecate_binding; neat! I just tried it and it works:) We could just do that 👍

@devmotion
Copy link
Member Author

I checked and deprecating Turing.Core with @deprecate_binding seems to work, the issue is still fixed.

@devmotion
Copy link
Member Author

I just tried it and it works:)

Hahahaha beat me again 😄

Co-authored-by: Tor Erlend Fjelde <[email protected]>
@codecov
Copy link

codecov bot commented Jan 14, 2022

Codecov Report

Merging #1762 (0ad1ef6) into master (5c4b4d5) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1762      +/-   ##
==========================================
- Coverage   81.29%   81.23%   -0.06%     
==========================================
  Files          24       24              
  Lines        1470     1471       +1     
==========================================
  Hits         1195     1195              
- Misses        275      276       +1     
Impacted Files Coverage Δ
src/Turing.jl 100.00% <ø> (ø)
src/essential/ad.jl 81.01% <ø> (ø)
src/essential/compat/reversediff.jl 95.34% <ø> (ø)
src/essential/container.jl 76.19% <ø> (ø)
src/essential/Essential.jl 100.00% <100.00%> (ø)
src/inference/AdvancedSMC.jl 97.43% <100.00%> (ø)
src/inference/Inference.jl 84.55% <100.00%> (ø)
src/inference/mh.jl 85.18% <0.00%> (-0.64%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac99fb0...0ad1ef6. Read the comment docs.

@devmotion devmotion merged commit 9087412 into master Jan 14, 2022
@delete-merged-branch delete-merged-branch bot deleted the dw/rename_core branch January 14, 2022 07:43
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.

UndefVarError: CodeInfo not defined

3 participants