natcap / invest

InVEST®: models that map and value the goods and services from nature that sustain and fulfill human life.
Apache License 2.0
164 stars 67 forks source link

Allow optional D8 or MFD routing in SDR and NDR #148

Closed richpsharp closed 5 months ago

richpsharp commented 4 years ago

SDR and NDR use MFD routing. Some researchers prefer D8 and some aren't sure but like to compare results against the two methods. This PEP would add:

phargogh commented 4 years ago

If we don't get to this before the 3.9 release, we'll bump this to 3.10.

richpsharp commented 4 years ago

FYI, in later discussion it wasn't so much that "natcap" wants D8 so much as it was an offhand in a meeting with a collaborator who didn't know what MFD was. And digging into that the real reason was that they didn't like MFD streams were multi pixels wide (even though they are more accurate). I've got a ticket to work on narrowing the MFD streams in response to this. My only thought is that if you do go through with this, that you make MFD the default and a warning that running in D8 will give inaccurate results that are non-linearly sensitive to error in the DEM?

In the future once we get the one pixel wide streams on MFD I'm hoping this option can be "MFD" or "constrain MFD stream pixels to be 1 pixel wide" and remove the option for D8 again.

dcdenu4 commented 4 years ago

Is it reasonable to remove the 3.9 release tag from this issue? Is there an argument the 3.9 release should wait for this to be implemented?

phargogh commented 4 years ago

Yep! Absolutely reasonable. I've removed this issue from the 3.9 release milestone.

After some discussion on the side, Rich's suspicion about narrower streams was confirmed, though there may also still be some value in supporting D8 for the sake of comparison as it's still the most commonly-used routing algorithm. Rich will prototype this in https://github.com/therealspring/inspring and we'll see what the hydrologists think about it.

richpsharp commented 3 years ago

So digging into this even more, turns out our "partners" were the World Bank and Conservation International. Neither of them wanted MFD because there's no hydrology literature published on it (aside from the InVEST model itself). So we rewrote NDR in inspring to go D8 only. It also has changes where it dropped the subsurface routing, allows for a custom load raster to be passed in that can be selected via certain landcover types, and it intentionally only operates on a specific watershed which makes it amenable to parallelization for global runs. link to ndr_plus for reference

Swapping MFD and D8 in and out isn't as trivial as swapping function calls since they both have some custom flow path walking in Cython modules underneath. So it's not so straightforward to do both without a lot of extra Cython code, plus you have to be careful about all the routing.

My recommendation for InVEST would be to leave it alone since the specific issue that caused this to be raised has been resolved and they couldn't use the InVEST NDR anyway since they wanted that custom load layer in it. If you were to do both there's the implementation complexity which can be handled of course, but it's also going to need some kind of scientific analysis of why and how the results are different (they are significantly different) and guidance about which one to use... also raises the question of which one is "right". Anyway, just raising all of that to note it's not a couple week kind of thing and the engagement with natcap science should be significant. Plus the motivation for needing it is resolved.

So I'm inclined to "wontfix" this issue, but lets chat tomorrow during our call and make sure that still makes sense.

dcdenu4 commented 3 years ago

Thanks for taking the time to provide a summary on this Rich.

emlys commented 2 years ago

@phargogh has partially completed this

newtpatrol commented 1 year ago

Resurrecting this D8 thing - I have been asking about adding D8 as an option to the freshwater models for years. The MFD algorithm may be better in some ways, but it also creates a lot of streams that are more like floodplains, obviously way too wide. This causes many problems:

This is coming to mind because I'm being asked about it by people I'm consulting - their modeled streams are often unrealistically wide. And I deal with this every time I use one of these models. Often we're lucky that it's not a huge deal, or I've worked around it in SDR by setting an extremely high TFA value (that creates no streams), then using a Drainages input that contains D8 streams created by RouteDEM. This workaround is only available in SDR, however.

Please add D8 as an option to all of the routed water models!

emlys commented 5 months ago

Closing as duplicate of #1440