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

Add matrix_apply_linter() #1869

Merged
merged 31 commits into from
Mar 29, 2023
Merged

Add matrix_apply_linter() #1869

merged 31 commits into from
Mar 29, 2023

Conversation

Bisaloo
Copy link
Contributor

@Bisaloo Bisaloo commented Dec 21, 2022

Fix #1764

To do:

  • Test lint messages more strictly
  • Add messages for simple cases when we have a matrix, or when l2 = length(dim(x))
  • Add NEWS item
  • Add linter tags
  • Add na.rm arg in reco if present in original call

Open questions for input:

  • I ended up implementing a linter for both colSums()/rowSums() and colMeans()/rowMeans(). The function should maybe have a more generic name but I'm not sure what
  • The case when MARGIN is passed as an integer is not covered but I believe this is not correct syntax anyways (should there be a linter for this?)
    apply(x, 1L, mean)
  • Detection of x, MARGIN and FUN is purely based on position. I don't believe it should be common to see deviation from this but I'm open to counterpoints

@Bisaloo Bisaloo marked this pull request as draft December 21, 2022 13:27
@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2022

Codecov Report

Merging #1869 (5b35252) into main (34abbf3) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 5b35252 differs from pull request most recent head f01d575. Consider uploading reports for the commit f01d575 to get more accurate results

@@            Coverage Diff             @@
##             main    #1869      +/-   ##
==========================================
+ Coverage   98.86%   98.88%   +0.01%     
==========================================
  Files         112      113       +1     
  Lines        4841     4915      +74     
==========================================
+ Hits         4786     4860      +74     
  Misses         55       55              
Impacted Files Coverage Δ
R/colsums_rowsums_linter.R 100.00% <100.00%> (ø)
R/indentation_linter.R 100.00% <0.00%> (ø)
R/implicit_assignment_linter.R 100.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Bisaloo Bisaloo marked this pull request as ready for review December 23, 2022 11:53
NEWS.md Outdated Show resolved Hide resolved
@AshesITR
Copy link
Collaborator

Regarding passing MARGIN as integer: we actually encourage this with implicit_integer_linter().

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Dec 24, 2022

Regarding passing MARGIN as integer: we actually encourage this with implicit_integer_linter().

Good point! I'm not sure to address that.

Since I'm doing operations on l1 and l2 to generate the custom error message, I can only think of solutions like eval(parse(text = margin)), which probably goes beyond the scope of static analysis.

@AshesITR
Copy link
Collaborator

You an use xml_text() instead of xml_integer() and then as.integer(re_substitutes(l1, "L$", "")) iinm.

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Dec 26, 2022

Done. The try(eval(parse(text = margin))) would also support cases like seq(1, 3), seq_len(3), c(1,2,3), etc. But I'm afraid we're stepping outside of the scope of lintr.

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Thanks, we're almost there.

R/colsums_rowsums_linter.R Outdated Show resolved Hide resolved
R/colsums_rowsums_linter.R Outdated Show resolved Hide resolved
tests/testthat/test-colsums_rowsums_linter.R Outdated Show resolved Hide resolved
tests/testthat/test-colsums_rowsums_linter.R Outdated Show resolved Hide resolved
R/colsums_rowsums_linter.R Outdated Show resolved Hide resolved
R/colsums_rowsums_linter.R Outdated Show resolved Hide resolved
@MichaelChirico
Copy link
Collaborator

The function should maybe have a more generic name but I'm not sure what

matrix_moments_linter() ?

@AshesITR
Copy link
Collaborator

AshesITR commented Jan 3, 2023

Future expansion of this linter would probably go towards adding more functions to the list of detected things (the matrixStats package has a few high-performance functions for rowwise / columnwise operations, e.g.)

Maybe matrix_apply_linter()?

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Jan 24, 2023

I like matrix_apply_linter() since "moments" might be unclear to non-statisticians.

@Bisaloo
Copy link
Contributor Author

Bisaloo commented Mar 28, 2023

Hi @AshesITR @IndrajeetPatil @MichaelChirico, is there anything left for me to do in this PR or are features freezed until the new release gets out? #1476

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing, I haven't had much spare time lately.
@MichaelChirico do we want to leave the name as matrix_apply_linter()?
If so, LGTM after fixing the news.

NEWS.md Outdated
@@ -100,6 +100,8 @@

### New linters

* `colsums_rowsums_linter()` recommends use of dedicated `rowSums()`, `colSums()`, `colMeans()`, `rowMeans()` over `apply(., MARGIN, sum)` or `apply(., MARGIN, mean)`. The recommended alternative is much more efficient and more readable (#1869, @Bisaloo).
Copy link
Collaborator

Choose a reason for hiding this comment

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

The name is wrong here.

@MichaelChirico
Copy link
Collaborator

@MichaelChirico do we want to leave the name as matrix_apply_linter()?

works for me! Thanks @Bisaloo for all the work on this & for your patience!

@Bisaloo Bisaloo changed the title Add colsums_rowsums_linter() Add matrix_apply_linter() Mar 29, 2023
Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

Thank you for your effort!

@AshesITR AshesITR merged commit 01500d8 into r-lib:main Mar 29, 2023
@Bisaloo Bisaloo deleted the colsums_rowsums branch March 30, 2023 09:00
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.

New lint_colsums_rowsums() linter
5 participants