Performance optimizations

[lochbika]

This set of commits replaces ifelse statements in a number of functions with explicit attributions through logical indexing. Additionally, the output data frame in the fwi function is pre-allocated now to avoid incremental c statements that concatenate the results in the loop.

This decreases the computation time from 90 seconds to 30 seconds for my test data set, which is a data frame with ~15,000 records.

The tests ran successfully (with one minor modification of test fwi_11)

[jordan-evens]

I think this makes sense and is worth doing. Some of those ifelse statements are to prevent division by zero, which this doesn't do for at least this: p <- ifelse(dmc == 0, 0, (dmc - bui1) / dmc)

p <- (dmc - bui1) / dmc
p[dmc == 0] <- 0

I think it would make more sense to either filter by value twice:

p[dmc != 0] <- (dmc - bui1) / dmc
p[dmc == 0] <- 0

Or assign everything to 0 and only apply the value that doesn't divide by zero

# I'm guessing it's something like this so it's not just assigning an integer to the variable?
values(p) <- 0
p[dmc != 0] <- (dmc - bui1) / dmc

Does that make sense to you, or is there a reason to not avoid dividing by 0 that I'm not seeing?

[lochbika]

Sure, to be on the safe side, I would chose your last suggestion with one addition. If dmc has a length > 1 then it wouldn't work. Plus, we also have to index the rhs part. So, I suggest the following (unless p,dmc will always have length 1)

p <- rep(0, length(dmc))
p[dmc != 0] <- (dmc[dmc != 0] - bui1[dmc != 0]) / dmc[dmc != 0]

What do you think?

[jordan-evens]

I think that makes sense. If all the tests are passing then I'd be happy to include this once we have that change in place. Thanks for your help.

[lochbika]

Done. All tests passed and R CMD CHECK reports no issues. No problem. Thanks for the great work on this project!

[jordan-evens]

Bear with me on this - I've never handled a PR before so I'm learning how this works as we go.

Tests run fine, and things generally look good.

Sorry to be picky but there are a few little things I'd like cleared up first - I was going to try to get to it but didn't manage to, so I'm wondering if you'd be able to fix them. Can you please go through all your changes and make sure all of the lines you're changing are required? I saw some things that looked redundant now that you're assigning to ranges from ranges.

We're updating dev and making sure that works before we put anything in main, and main only gets updated when we update CRAN. I changed the branch to dev, but that seems to pull dev into your changes so I'm wondering if you could rebase your branch to be off of dev on your end?

Also, I see another commit in your repo to fix the div by 0 issue, but it doesn't seem to be in the list of what this PR wants to include, so I'm wondering if you need to update the PR to point to that new commit? I would have thought pointing it at your main branch would be enough since it's there, but maybe it pick the revision that was there when you make the PR in case it changes later? Is there some way for you to update what commit this PR is pointing at?

[lochbika]

No worries, we don't want to rush it. Ok, I will check back and go through all the changes to make sure that there is no unnecessary/redundant code. I will also figure out how I can move the commits to the dev branch. Could be, however, that I have to re-create the pull request then from a clean starting point on the dev branch. The number of changed files is small, though. So, it shouldn't be a problem.

I'll be on holidays for the next week and do all these changes when I'm back.

[jordan-evens]

Sounds good. Again, thanks for your help on this. If you need to or it's easier to submit a new PR then that's not an issue on my end.

[lochbika]

Ok, I'm back. I will approach this the following way. I will try to move the changes to the dev branch and open a new pull request. Fingers crossed :crossed_fingers:

[lochbika]

This will be continued in PR #42