Remove `tonamedtuple` #547

sunxd3 · 2023-10-23T10:28:03Z

sunxd3 · 2023-10-23T10:31:48Z

I support the idea of removing all the tonamedtuple function definitions. But I am not sure how to do it properly. Can we just remove the function definitions? I would assume that counts as a breaking change and/or we should deprecate?

codecov · 2023-10-23T10:52:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2e8adf4) 82.85% compared to head (1bdc4ea) 80.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #547      +/-   ##
==========================================
- Coverage   82.85%   80.22%   -2.63%     
==========================================
  Files          25       25              
  Lines        3184     3156      -28     
==========================================
- Hits         2638     2532     -106     
- Misses        546      624      +78

Files	Coverage Δ
src/DynamicPPL.jl	`33.33% <ø> (ø)`
src/abstract_varinfo.jl	`81.41% <ø> (-4.43%)`	⬇️
src/simple_varinfo.jl	`83.70% <ø> (+8.45%)`	⬆️
src/threadsafe.jl	`46.01% <ø> (+0.40%)`	⬆️
src/varinfo.jl	`86.64% <ø> (-6.68%)`	⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yebai · 2023-10-23T11:04:39Z

I think it is okay to remove the function definitions since we no longer use them in Turing. @torfjelde can probably confirm this.

torfjelde · 2023-10-23T16:53:35Z

@sunxd3 IIRC I've removed all usages of tonamedtuple in Turing.jl now, but can you check this?

Also, note that this would have to be a breaking release since tonamedtuple is an exported function. And because of this, let's wait with merging this PR until outstanding non-breaking PRs have been merged, e.g. #546 #548 . Once those are through, we can merge this PR + #541 together in a breaking release 👍

sunxd3 · 2023-10-23T17:27:16Z

@torfjelde I did a repo-wide word search for tonamedtuple and none showed up.
Also, agree on that this should be breaking.

torfjelde · 2023-10-23T21:51:51Z

Great! Then bump the minor version please:)

torfjelde · 2023-10-25T22:53:51Z

This is a breaking change @yebai but we made #541 non-breaking, so we need to merge that + release before we merge this.

github-actions · 2023-10-26T18:29:20Z

Pull Request Test Coverage Report for Build 6658081508

0 of 0 changed or added relevant lines in 0 files are covered.
102 unchanged lines in 10 files lost coverage.
Overall coverage decreased (-2.6%) to 80.228%

Files with Coverage Reduction	New Missed Lines	%
src/sampler.jl	1	90.0%
src/threadsafe.jl	1	46.02%
src/contexts.jl	2	77.27%
src/logdensityfunction.jl	2	55.56%
src/prob_macro.jl	4	84.67%
src/abstract_varinfo.jl	5	81.42%
src/context_implementations.jl	5	60.23%
src/model_utils.jl	10	19.64%
src/loglikelihoods.jl	16	54.84%
src/varinfo.jl	56	86.64%

Totals
Change from base Build 6648358193:	-2.6%
Covered Lines:	2532
Relevant Lines:	3156

💛 - Coveralls

…ep existing compat) (#553) * Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models (#541) * preserve context from model in `rand` * replace rand overloads in TestUtils with definitions of rand_prior_true so we can properly test rand * removed NamedTuple from signature of TestUtils.rand_prior_true * updated references to previous overloads of rand to now use rand_prior_true * test rand for DEMO_MODELS * formatting * fixed tests for rand for DEMO_MODELS * fixed linkning tests * added missing impl of rand_prior_true for demo_static_transformation * formatting * fixed rand_prior_true for demo_static_transformation * bump minor version as this will be breaking * bump patch version * fixed old usage of rand * Update test/varinfo.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed another usage of rand --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Remove `tonamedtuple` (#547) * Remove dependencies to `tonamedtuple` * Remove `tonamedtuple`s * Minor version bump --------- Co-authored-by: Hong Ge <[email protected]> * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) --------- Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: CompatHelper Julia <[email protected]>

…t) (#551) * CompatHelper: bump compat for AbstractMCMC to 5, (keep existing compat) * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) (#552) Co-authored-by: CompatHelper Julia <[email protected]> * Update to AbstractMCMC 5 * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Update sampler.jl * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) (#553) * Fix for `rand` + replace overloads of `rand` with `rand_prior_true` for testing models (#541) * preserve context from model in `rand` * replace rand overloads in TestUtils with definitions of rand_prior_true so we can properly test rand * removed NamedTuple from signature of TestUtils.rand_prior_true * updated references to previous overloads of rand to now use rand_prior_true * test rand for DEMO_MODELS * formatting * fixed tests for rand for DEMO_MODELS * fixed linkning tests * added missing impl of rand_prior_true for demo_static_transformation * formatting * fixed rand_prior_true for demo_static_transformation * bump minor version as this will be breaking * bump patch version * fixed old usage of rand * Update test/varinfo.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed another usage of rand --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Remove `tonamedtuple` (#547) * Remove dependencies to `tonamedtuple` * Remove `tonamedtuple`s * Minor version bump --------- Co-authored-by: Hong Ge <[email protected]> * CompatHelper: bump compat for AbstractMCMC to 5 for package test, (keep existing compat) --------- Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: CompatHelper Julia <[email protected]> * bump AbstractPPL version to 0.7 * Update AbstractPPL test dependency * add `Random.AbstractRNG` * Update sampler.jl (#557) * Update src/sampler.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --------- Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> Co-authored-by: Tor Erlend Fjelde <[email protected]> Co-authored-by: Xianda Sun <[email protected]> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Xianda Sun <[email protected]>

Remove dependencies to tonamedtuple

8d079be

Remove tonamedtuples

e4cb1ae

yebai approved these changes Oct 23, 2023

View reviewed changes

yebai enabled auto-merge October 23, 2023 16:16

torfjelde disabled auto-merge October 23, 2023 16:53

sunxd3 and others added 2 commits October 24, 2023 07:15

Minor version bump

324cb8e

Merge branch 'master' into sunxd/remove_tonamedtuple

cea3ed9

yebai enabled auto-merge October 25, 2023 21:27

yebai added this pull request to the merge queue Oct 25, 2023

torfjelde removed this pull request from the merge queue due to a manual request Oct 25, 2023

Merge branch 'master' into sunxd/remove_tonamedtuple

1bdc4ea

yebai enabled auto-merge October 26, 2023 18:09

yebai added this pull request to the merge queue Oct 26, 2023

Merged via the queue into master with commit 04b03cd Oct 26, 2023
11 of 13 checks passed

yebai deleted the sunxd/remove_tonamedtuple branch October 26, 2023 18:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `tonamedtuple` #547

Remove `tonamedtuple` #547

sunxd3 commented Oct 23, 2023

sunxd3 commented Oct 23, 2023

codecov bot commented Oct 23, 2023 •

edited

Loading

yebai commented Oct 23, 2023

torfjelde commented Oct 23, 2023

sunxd3 commented Oct 23, 2023

torfjelde commented Oct 23, 2023

torfjelde commented Oct 25, 2023

github-actions bot commented Oct 26, 2023

Remove tonamedtuple #547

Remove tonamedtuple #547

Conversation

sunxd3 commented Oct 23, 2023

sunxd3 commented Oct 23, 2023

codecov bot commented Oct 23, 2023 • edited Loading

Codecov Report

yebai commented Oct 23, 2023

torfjelde commented Oct 23, 2023

sunxd3 commented Oct 23, 2023

torfjelde commented Oct 23, 2023

torfjelde commented Oct 25, 2023

github-actions bot commented Oct 26, 2023

Pull Request Test Coverage Report for Build 6658081508

💛 - Coveralls

Remove `tonamedtuple` #547

Remove `tonamedtuple` #547

codecov bot commented Oct 23, 2023 •

edited

Loading