search

cffdrs / cffdrs_r

FWI and FBP components of CFFDRS in R
GNU General Public License v2.0
11 stars 5 forks source link

Performance optimizations (redux) #42

Closed lochbika closed 1 month ago

lochbika commented 2 months ago

This is a fixed version of PR #41

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)

lochbika commented 2 months ago

Now the branches are set to dev in both repos. I will start the code review of my changes one of these days.

jordan-evens commented 1 month ago

Sorry is this the version you wanted to merge as it is? You comment I will start the code review of my changes one of these days. made me think you still had some stuff you wanted to change but that hasn't happened so I'm wondering if I misinterpreted what you were saying.

lochbika commented 1 month ago

Hi, first, sorry for the delay with this, times have been busier than I expected.

From my side the code works and produces the same results as the CRAN version for my test data set. The code review comment from my side was due to your request to check my changes for redundancy: 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.

This given, I think it is worth double checking before committing this to CRAN. I will check my changes for potential issues until the end of the week.

Best

BadgerOnABike commented 1 month ago

Looks good to me, I'm wondering if there are ways to further optimize the if() as in if there is an in loop variable we can use instead of recalling !prec_sel for instance. For now this is more than fine though and I'll continue to dig around and see what else can continue to be optimized.

Thank you so much for your effort here, this is really helpful!

lochbika commented 1 month ago

Excellent, thanks. I not sure how much the performance increase will be with further optimizing at the moment. I used a profiler to identify the most significant bottlenecks. But of course, there are definitely more places in the code where a bit more performance can be squeezed out.