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

New behaviour of droplevels in Rdevel #193

Closed
gavinsimpson opened this issue Aug 21, 2016 · 4 comments
Closed

New behaviour of droplevels in Rdevel #193

gavinsimpson opened this issue Aug 21, 2016 · 4 comments
Assignees
Milestone

Comments

@gavinsimpson
Copy link
Contributor

@gavinsimpson gavinsimpson commented Aug 21, 2016

R Core have altered the behaviour of droplevels(). For some initial discussions see #192. The basic issue is that now droplevels is defined as factor(x, ..., exclude = NULL) and this coupled with changes to factor() when exclude = NULL mean behaviour may have changed. To maintain behaviour of R v3.3 we should use droplevels(x, exclude = NA). This issue exists to track changes to the range of functions in vegan that use droplevels.

Current list of functions known to use droplevels

  • centroids.cca
  • envfit.default
  • factorfit
  • betadisper (Fixed in ae67eba, see #192)
  • hiersimu.default
  • adipart.default
  • multipart.default

I'm tracking this on the issue-#193 branch.

@gavinsimpson gavinsimpson added this to the 2.4-1 milestone Aug 21, 2016
gavinsimpson added a commit that referenced this issue Aug 21, 2016
@gavinsimpson gavinsimpson self-assigned this Aug 21, 2016
gavinsimpson added a commit that referenced this issue Aug 21, 2016
gavinsimpson added a commit that referenced this issue Aug 22, 2016
@gavinsimpson gavinsimpson changed the title New behaviour of droplevels (and factor) in Rdevel New behaviour of droplevels in Rdevel Aug 22, 2016
@jarioksa
Copy link
Contributor

@jarioksa jarioksa commented Aug 22, 2016

It seems that droplevels has not been stabilized yet, but discussion continues. Probably we need to wait till droplevels() stabilizes.

@gavinsimpson
Copy link
Contributor Author

@gavinsimpson gavinsimpson commented Aug 22, 2016

Right; I was in two minds whether to continue this when I saw the R-devel thread. However, the proposed changes don't break anything nor rely on whatever behaviour RCore ultimately settles upon, but they do make explicit the behaviour we want/expect/anticipate and so should be robust to RCore tweaking.

I would suggest we don't push out a 2.4-1 just for this though if we don't continue to generate errors when R-devel version settles down. We can hold off till a future 2.5-0 release if that is preferable?

@jarioksa
Copy link
Contributor

@jarioksa jarioksa commented Aug 23, 2016

It seems that droplevels() behaves now similarly as earlier, and vegan_2.4-0 passes R CMD check without problems. The change was made in this commit:

r71129 | maechler | 2016-08-22 16:40:07 +0300 (Mon, 22 Aug 2016) | 1 line
Changed paths:
   M /trunk/src/library/base/R/factor.R
   M /trunk/src/library/base/man/droplevels.Rd
   M /trunk/tests/reg-tests-1c.R

droplevels() w/ smarter `exclude` default, now compatible to `[, drop=TRUE`

From that point of view, we need not push a new minor release. However, there are other bug fixes that may call for a minor release like discussed in issue #194.

gavinsimpson added a commit that referenced this issue Sep 3, 2016
gavinsimpson added a commit that referenced this issue Sep 3, 2016
@gavinsimpson
Copy link
Contributor Author

@gavinsimpson gavinsimpson commented Sep 3, 2016

Merged into master after merging typo. Closing.

jarioksa added a commit that referenced this issue Sep 4, 2016
jarioksa added a commit that referenced this issue Sep 4, 2016
jarioksa added a commit that referenced this issue Sep 4, 2016
jarioksa added a commit that referenced this issue Sep 4, 2016
jarioksa added a commit that referenced this issue Sep 4, 2016
jarioksa added a commit that referenced this issue Sep 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.