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

i) no := recycling ii) some INTEGER()-etc moved up #3310

Merged
merged 10 commits into from
Feb 3, 2019
Merged

Conversation

mattdowle
Copy link
Member

@mattdowle mattdowle commented Jan 24, 2019

Initially this branch was for #3301. The memrecycle internals needed attention for that so I decided to tackle := recycling at the same time. All the changes to tests look much better without recycling I think.
13 CRAN revdeps affected, plus 3 Bioconductor. Hopefully these are mostly unintended bugs that the maintainers will be glad to find.

Output for the 16 out of 748 : fail.log

Tick below means maintainer replied to email saying they agree in principle. After consultation, this PR was merged and #3347 was filed to track PRs we'll submit to those package.

  • antaresViz
  • expss
  • iml
  • irg
  • popEpi
  • rAmCharts
  • riskRegression
  • scopr
  • sdcTable
  • simstudy
  • vardpoor
  • multistateutils
  • POUMM

plus 3 out of 152 Bioconductor packages :

  • dada2
  • maftools
  • metagene

@mattdowle mattdowle added this to the 1.12.2 milestone Jan 24, 2019
NEWS.md Outdated
@@ -4,6 +4,8 @@

#### NEW FEATURES

1. `:=` no longer recycles too-short RHS vectors to match the LHS length. There was a warning when recycling left a remainder but no warning when the LHS length was an exact multiple of the RHS length (the same behaviour as base R). All recycling is now an error unless `length(RHS)==1` or `length(RHS)==length(LHS)`. Consistent feedback was that recycling is rarely useful and more often a bug. In rare cases where you need to recycle a length>1 vector, use `rep()` explicitly. Single values are still recycled silently as before.
Copy link
Member

Choose a reason for hiding this comment

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

might be worth doing a revdep run with this commit, i'm curious how many dependencies assume this behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

Running. First 80 of 750 complete. Just 1 affected so far: antaresViz

Copy link
Member Author

Choose a reason for hiding this comment

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

Complete. 16 affected out of 748. I've updated the top comment.

@codecov
Copy link

codecov bot commented Jan 24, 2019

Codecov Report

Merging #3310 into master will increase coverage by <.01%.
The diff coverage is 98.24%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3310      +/-   ##
==========================================
+ Coverage    94.8%   94.81%   +<.01%     
==========================================
  Files          65       65              
  Lines       12094    12104      +10     
==========================================
+ Hits        11466    11476      +10     
  Misses        628      628
Impacted Files Coverage Δ
src/reorder.c 97.43% <100%> (ø) ⬆️
src/subset.c 100% <100%> (ø) ⬆️
src/dogroups.c 93.29% <97.82%> (-0.07%) ⬇️
src/assign.c 94.83% <98.27%> (+0.16%) ⬆️
src/wrappers.c 95.58% <0%> (-0.13%) ⬇️

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 0e5fb5c...b72382e. Read the comment docs.

@codecov
Copy link

codecov bot commented Jan 24, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d3e32c2). Click here to learn what that means.
The diff coverage is 98.24%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3310   +/-   ##
=========================================
  Coverage          ?   94.81%           
=========================================
  Files             ?       65           
  Lines             ?    12104           
  Branches          ?        0           
=========================================
  Hits              ?    11476           
  Misses            ?      628           
  Partials          ?        0
Impacted Files Coverage Δ
src/reorder.c 97.43% <100%> (ø)
src/subset.c 100% <100%> (ø)
src/dogroups.c 93.29% <97.82%> (ø)
src/assign.c 94.83% <98.27%> (ø)

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 d3e32c2...92b785a. Read the comment docs.

@mattdowle mattdowle changed the title Remove USE_RINTERNALS and DATAPTR i) no := recycling ii) some INTEGER()-etc moved up Feb 3, 2019
@mattdowle mattdowle mentioned this pull request Feb 3, 2019
16 tasks
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.

2 participants