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

Use !anyNA() over all(!is.na(.)) #500

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

MichaelChirico
Copy link
Contributor

all(!(.)) is logically equivalent to !any(.); in this case, that means we can use the specialized anyNA() instead as well

`all(!(.))` is logically equivalent to `!any(.)`; in this case, that means we can use the specialized `anyNA()` instead as well
@gavinsimpson
Copy link
Contributor

There's also an instance in R/ordConstrained.R

if (all(!is.na(wa)))

Does this make a practical difference? - Are there examples of this being a performance bottleneck?

@MichaelChirico
Copy link
Contributor Author

to me it's about readability as much as performance -- I find "aggregate + negate" easier to process than "negate + aggregate".

Came across it because our (very) old version was tripping the previous error from if (!is.na(wa)) when length(wa)>1, not for performance reasons.

@gavinsimpson gavinsimpson self-assigned this Mar 9, 2022
@gavinsimpson
Copy link
Contributor

OK - I'll deal with this and the extra instance I found (plus a more thorough check for other instances than the GH search I did to find the ordConstrained one).

Thanks :-)

@MichaelChirico
Copy link
Contributor Author

MichaelChirico commented Mar 9, 2022

FWIW this will eventually be available as the OuterNegationLinter:

r-lib/lintr#884

the problem with writing all those linters is now I can't help but see them everywhere :)

(PS in the meantime, I can run it manually and file PRs on any package if you'd like)

@gavinsimpson gavinsimpson merged commit b6ea455 into vegandevs:master Mar 9, 2022
@gavinsimpson
Copy link
Contributor

Thanks @MichaelChirico

@MichaelChirico MichaelChirico deleted the patch-1 branch March 9, 2022 10:25
@jarioksa
Copy link
Contributor

jarioksa commented Mar 10, 2022

Just a historic note: anyNA was introduced in R version 3.1.0 in April 2014, and the code you changed is from 2001. I think this is "syntactic sugar", although the docs say that

The default method for anyNA handles atomic vectors without a
class and NULL. It calls any(is.na(x)) on objects with
classes and for recursive = FALSE, on lists and pairlists.

It is more readable, though. The release note says that it may be faster for atomic vectors (without a class).

@gavinsimpson
Copy link
Contributor

Yep; I triggered the GH Actions checks for the PR to insure that we weren't inadvertently bumping the minimum version of R we require.

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.

3 participants