BUMBANUMBA

[ericbarefoot]

Numba speedup

I was interested in what @amoodie did in #27, so I started messing around with it too once #29 was merged. In particular, with more modular functions, how could a lot of things be moved into a utils-like module, jitted, and then called that way.

I basically took a profile of the code, and just started jitting the things that took the longest, to see how fast we could get. So far, I have pulled out and jitted the following:

• the construction of the water weight array
• calculating the next step indices for water
• updating the water flow field.
• checking for loops in the water paths
• the random pick function
• choosing weights for sediment routing
• spreading sand between cells.

There are quite a few more things that take a significant fraction of the computational time, and could also go into jitted functions. However, I'm stopping here for now, because with the default parameters, I see about a 25x speedup from the develop branch with these adjustments. Using this script, I get about 9.54s per iteration for develop, and 0.36s per iteration after jitted functions are compiled.

from pyDeltaRCM.deltaRCM_driver import pyDeltaRCM
import time

delta = pyDeltaRCM()

start1 = time.time()
delta.update()
end1 = time.time()
print("Elapsed time for first iteration including compile = %s" % str((end1 - start1)))

n = 10
start = time.time()
for _t in range(0, n):
    delta.update()
    print("timestep:", _t)

end = time.time()

print("Elapsed time per iteration with jitting = %s" % str((end - start) / n))

Needs

I'd like y'all's feedback on the organization. Basically, I'm not sure about placing these things in a separate utils, so maybe you can comment @amoodie, if it would be good to move the things that belong to water into the water_tools file, but have them as standalone jitted functions in the water_tools module.

Some other ideas I have that may make this organization more intuitive and the long function calls more palatable:

• rename shared_tools to utils like @amoodie did
• move some commonly used attributes of the classes that are not set by the user to the utils module as getter functions. I kinda did this for the self.iwalk and self.jwalk attributes, which are initialized now using a getter function from shared_tools. The idea is that if these getters become compiled themselves, we could have them called inside jitted functions and not have to supply them in the arguments.
• move jitted functions unique to sed and water routing into the module where these classes are defined.

Thoughts??

[ericbarefoot]

Ok. I also need some advice with the docs, because that's what failed the second time around. How should I fix that?

[ericbarefoot]

I marked this as a draft because of two things. I will have to learn a little bit more about the summary for modules so that the module with the jitted functions becomes documented. Also, it looks like this branch is not producing the correct results. I just looked at a picture of the results two steps in, and something is different than develop. However, I still want feedback on other stuff while I hunt down these bugs.

[amoodie]

Hi @ericbarefoot, awesome effort.

Also, it looks like this branch is not producing the correct results. I just looked at a picture of the results two steps in, and something is different than develop.

Yeah, I can verify this branch produces different results, based on my PR for a consistency test. I thought maybe this was because your branch uses a non-jitted pick_random_inlet function, which would change the number of random calls, but this does not appear to be the case (see below).

I don't think it matters what you decide to name the file with all these new jitted functions. It could make sense to change the name since shared_tools is no longer inherited by Tools like all of the other _tools methods. I think shared would a fine and general-enough module name.

I'm gonna send you a PR now that jits the pick_random_inlet and fixes the documentation with all the new jitted functions you have added. If you change the name of shared_tools, just update the doc folder structure and files accordingly. You can always test the docs locally with (cd docs && make test) from the root folder, or test and build the docs with (cd docs && make docs) (see the targets in the Makefile).

[amoodie]

hey @ericbarefoot, one issue might be that the changes you made to random_pick now allow the choice to the same cell (i.e., the center of the 3x3 neighborhood). I think before, when the method took an 8 cell array, the choice returned was never the same cell as current.

Maybe you can try to revert this behavior and see if you get a consistent result with develop? You'll need to use my jitted random_pick_inlet too, for sure.

Nevermind, I think that part actually is fine lol

[ericbarefoot]

Hey @amoodie I just went spelunking to find the bugs that were messing with water and sediment routing. I found and fixed them, but it affects some of the things in your PR, which I didn't want to change further to find the problem.

Can you rebase your branch on ef18f96? This is a rebase of the changes I made on the new stuff in develop. That way I can easily include your fixes to the docs and the other list typing stuff.

[ericbarefoot]

Hey all: I think this is ready to go now. I wrote a whole bunch of tests for the new jitted functions today. Turns out, the coveralls doesn't say lines are "covered" if the code is compiled prior to running, so we can think about how to address that if you all would like to make sure we continue to increase coverage always.

[amoodie]

I tried messing around with an environment variable to the coverage job on travis (NUMBA_DISABLE_JIT) I thought would fix this coverage issue, but apparently it does not, based on some comments around github. I think we could open an issue to keep checking on it and see if a fix comes along.

Based on 092d98c, you were able to fix the bug without changing the test_consistent? Nice. And did you run that yaml configuration I had given you to verify the bug is resolved?

Otherwise, looks good to me 👍

[ericbarefoot]

Based on 092d98c, you were able to fix the bug without changing the test_consistent?

As far as I can tell, it's close enough yeah. I was surprised too.

And did you run that yaml configuration I had given you to verify the bug is resolved?

Yup. no more strange zig zagging paths, and it makes it through without erroring out.

[amoodie]

As far as I can tell, it's close enough yeah. I was surprised too.

Thinking about it a bit more, I guess it makes sense. The test never had the bug (only ran for one small timestep), so why would it be affected by the bug being fixed‽

Yup. no more strange zig zagging paths, and it makes it through without erroring out.

Cool, anyway yeah lgtm, not sure if Jay wants to look or not.

[elbeejay]

Great effort! This is all really clean and easy to follow.

More a note than any commentary on this PR, but in general we should probably try running longer "test" simulations for large changes to check for longer-term stability as those types of checks are not going to be possible via unit testing or any continuous integration method.

[elbeejay]

If iwalk and jwalk are moved to shared_tools (which is fine by me) does it make sense to also move over all of the other walk/vector arrays like jvec and so forth?

[ericbarefoot]

Yeah, I think I see a larger 'reorganize and tidy' PR on the horizon, where we once again move stuff into separate classes. I think this relates to the milestones for splitting out the hydrodynamics and morphodynamics code into separate modular classes, with a utils or similar module that they both access. I think moving a bunch of the stuff like the definitions of these arrays into something like utils makes a lot of sense.

[elbeejay]

Oh, and regarding the coverage not including compiled code I also did a cursory search and did not find a solution that looks like it will work for us. The closest thing I found was this, which pretty much required writing a numba toggle for each function and testing them with numba turned off to "cover" the code in the unit tests. For this project I don't know of a good reason why one would want to turn numba off so I think a toggle would just clutter things up. So I'm good on the coverage front as well.