Change weight calculations for water and sediment routing

[ericbarefoot]

Overview

This is a rival to #27, where I implement some things that @amoodie and I have also talked about, but do no jit-ting on the code. Instead, I basically took the perspective that calculating the probability weights for neighbors of a cell is too expensive to do every step of the way, so if possible, it should be computed beforehand. For the water calculations, this makes a lot of sense, since the probability field for water only depends on the surface and the inertial component of flow. These are updated five times throughout every timestep, and 2000 water parcels route using the same field. Here, I move it to the top of every water iteration loop, and pre-compute probabilities for the whole array. I also took some of the process for selecting the random pick into the weighting function itself, so that no sums need to be calculated to pick.

This kind of scheme isn't possible with the sediment, because erosion and deposition change the probability field with every step, so unfortunately, we have to calculate weights on the fly. I made the random pick function compatible across both too.

Speedup

The net result of this is a pretty hefty speedup, since so much computation time is spent moving water around. These little tweaks give me ~ a 50% speedup. Using @amoodie's script:

from pyDeltaRCM.deltaRCM_driver import pyDeltaRCM
import time

delta = pyDeltaRCM()
start = time.time()

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

end = time.time()

I get about 2:45 min on my old-a$$ laptop when I run on our develop. I get about 1:10 min with this PR.

I think this and #27 are compatible. One thing to consider, is that I think numba has a @jitclass decorator, and we could maybe do that with utils or something?

Small sidenote: @amoodie, I implemented one aspect of #28, just changing the default yaml. If you'd like to, I'm happy to incorporate the other stuff here too, but wanted feedback on this, rather than that.

[ericbarefoot]

Also, I did no documentation or new tests. Again, like #27 this is meant to be for feedback, not merging as-is.

[amoodie]

Coolio, I think it makes sense to do this before any jitting. It'll be no problem to implement that after.

1. Can you add the test_output_consistency file from my PR to yours? This will check that your refactoring gives the exact same output as the status of develop you are PRing against.
2. Assuming that passes, I'm good with you merging this and jitting later
3. I think eta figs should be on by default, and discharge off, but I'm not gonna fight about it.

[amoodie]

fix: bug where topo diffusion happened after mud

ahh so you changed functionality, so my suggestion above about the testing will fail, I think. I see this is related to #9, so I guess we want to make the change, and I can remake the test file in the future.

[elbeejay]

fix: bug where topo diffusion happened after mud

ahh so you changed functionality, so my suggestion above about the testing will fail, I think. I see this is related to #9, so I guess we want to make the change, and I can remake the test file in the future.

I am a bit behind on some of the "new" changes you all are proposing but will be catching up soon. But with regards to this, I was never sure if that change in order was made intentionally to solve some other instability or what. It seems like it must have been done intentionally.

[ericbarefoot]

I am a bit behind on some of the "new" changes you all are proposing but will be catching up soon. But with regards to this, I was never sure if that change in order was made intentionally to solve some other instability or what. It seems like it must have been done intentionally.

I actually just made this change based on the documentation. I'm fine to move it back actually. Let's put this in a different issue and I can scrub this from this PR. I think a new issue, and some subsequent work would be good to figure if this should be here or not.

[ericbarefoot]

I think eta figs should be on by default, and discharge off, but I'm not gonna fight about it.

What do you think @elbeejay ?

[ericbarefoot]

ahh so you changed functionality, so my suggestion above about the testing will fail, I think. I see this is related to #9, so I guess we want to make the change, and I can remake the test file in the future.

Interesting. I tried changing it back, and it fails anyway. Let me go commit by commit and see if I can figure out what changes it.

EDIT: the test in the consistency check fails on my develop @amoodie.

[elbeejay]

I think eta figs should be on by default, and discharge off, but I'm not gonna fight about it.

What do you think @elbeejay ?

I have no preference either way. I can see the value in both proposed solutions. It is such a small thing for someone to change to suit their needs anyway.

With regards to the routing and order of diffusion, there doesn't seem to be any explanation for the change. Topographic diffusion used to be implemented after the routing of coarse particles al la the Matlab order of operations (old commit). But after the topo_diffusion function was broken out separately, it just migrated to being called after all sediment routing had been performed.

[amoodie]

EDIT: the test in the consistency check fails on my develop @amoodie.

lol yeah it's failing on mine now too 👀

The more I think about it though, based on what you said in your now edited comment about the number of times random is called, it's probably a bad test. I can try to write another small, similar tests on develop and we can see if it works, but 'm fine with moving forward with your PR now

[ericbarefoot]

Groovy, so I'll go ahead and merge this, then we can think about integrating numba for additional speedup for fun and profit.