initial setup for refactoring size processing to separate eigen-specific steps. - #425
Conversation
| } | ||
|
|
||
| if(nimbleOptions('useRefactoredSizeProcessing')) { | ||
| exprClasses_setToEigenize(compileInfo$nimExpr, compileInfo$newLocalSymTab, compileInfo$typeEnv) |
There was a problem hiding this comment.
Is this the one place where we will eventually toggle based on nimbleOptions('useTensorflow')?
There was a problem hiding this comment.
At the moment, yes. I think it will get slightly more complicated: If lifting temporaries out of expressions happens at later steps, we'll need to manage which pathway of catch-up processing needs to happen on those temporaries.
# Conflicts: # packages/nimble/R/genCpp_exprClass.R # packages/nimble/R/genCpp_sizeProcessing.R # run_tests.R
| browser() | ||
| } | ||
|
|
||
| if(nimbleOptions('useRefactoredSizeProcessing')) { |
There was a problem hiding this comment.
It's a good habit to name these short-lived flags experimentalMyFlagName or internalMyFlagName to make it clear to our users that they shouldn't use these flags.
| if(reset) { | ||
| nextIndex <<- 1 | ||
| return(invisible(NULL)) | ||
| labelFunctionMetaCreator <- function() { |
There was a problem hiding this comment.
Add comment describing the purpose of this function.
| } | ||
|
|
||
| sizeDiagonal <- function(code, symTab, typeEnv) { | ||
| ## refactor: code$name change step stays here |
There was a problem hiding this comment.
nit: If you label your comments useRefactoredSizeProcessing, you will have an easier time systematically cleaning up those comments when you're done.
| @@ -0,0 +1,197 @@ | |||
| 1### INSTRUCTIONS: | |||
There was a problem hiding this comment.
Thankyou for adding instructions! 👍
|
|
||
| ## Known concern: ordering of asRow()/asCol() and intermediates | ||
|
|
||
| compareOldAndNewCompilationRC <- function(input) { |
There was a problem hiding this comment.
It would be nice to have unit tests, rather than these tests that "this unit behaves the same as it used to".
|
|
||
| # Avoid running these blacklisted tests, since they take too long. | ||
| blacklist <- c('test-Math2.R', 'test-Mcmc2.R', 'test-Mcmc3.R', 'test-Filtering2.R', 'test-ADfunctions.R', 'test-ADmodels.R') | ||
| blacklist <- c('test-Math2.R', 'test-Mcmc2.R', 'test-Mcmc3.R', 'test-Filtering2.R', 'test-ADfunctions.R', 'test-ADmodels.R','test-refactorSizeProcessing.R') |
There was a problem hiding this comment.
Why is the test blacklisted? Does it currently fail? This knowledge deserves to live as a comment in the code:
blacklist <- c(blacklist, 'test-refactorSizeProcessing.R') # These tests of an experimental refactoring currently fail.
|
Thanks for the code review @fritzo ! I've updated for all points except the larger step of adding different flavors of tests. If tests pass I think this is ready to merge. |
|
👍 |
2 participants
This PR is ready to merge if it passes tests.
This PR include basic setup for refactoring size processing to disentangle eigen-specific steps from general size and type annotation steps. Eigen-specific steps include annotation of what can be handled by eigen and generating intermediates where needed for implementation using eigen. Separating these steps into distinct compiler passes should make the code cleaner and more amenable to extension such as for other back-ends.
Currently the experimental size processing works for everything in
basicMathTests.Changes include:
Two new compiler flags that toggle new behavior (default off):
experimentalNewSizeProcessingandexperimentalSelfLiftStage.A new compiler stage,
exprClasses_toEigenize, modeled onexprClasses_setSize, with handlers partially set up for simple cases, with placeholders for non-simple cases.A new test file. This includes a function that will compare whether old and new generated C++ files are identical for a given RCfunction. It currently has one toy test for that function. It also includes a function that wraps a call to
test_mathand checks whether current and experimental processing of each case inbasicMathTestsis identical. That passes.