search

HARPgroup / HARParchive

This repo houses HARP code development items, resources, and intermediate work products.
1 stars 0 forks source link

Preliminary fix of upstream storage? #1216

Closed COBrogan closed 2 months ago

COBrogan commented 3 months ago

Made some changes to how we find upstream segments. Also changed some minor code to work with R 4.3.3 e.g. changes to window. Also vectorized an if-then statement for additional clarity. Issue remains in how to find upstream segments. This method is very slow, but solves the problem of missing upstream storage. See #1213

glenncampagna commented 3 months ago

The fix for upstream storage works well, and I see your annotations and changes throughout the code which I greatly appreciate. I made a 1-line change to fix a bug coming from the master branch to assign the element id correctly

rburghol commented 3 months ago

Hey @COBrogan thanks for creating this PR nd using the review request function -- it helped me find it! Your initial retrieval of the list of river segments definitely is what fixes this behavior, and it solves 100% of the problem if we just use it as a left outer join against the model results -- so note that the bug is not in fn_extract_basin() -- the remainder of the code changes (which I think are causin the slow down) don't need to be there.

Couple comments about river segment retrieval: I see a cople downsides to pulling from the View using fn_download_read():

  1. That View doesn't discern model version (or if the segment has a model), nor ftype, which is why we use om_vahydro_metric_grid() -- as it discerns, otherwise we are going to get way too many features (more an issue with ftype, not as much model version, see below).
  2. fn_download_read() is not secure, i.e. we could go to that URL/View, but need to use ds$auth_read() for the same URL (and close down the permission on that view).

Neither my first or my second objection or dealbreaker of themselves, but om_vahydro_metric_grid() should contains all of the above -- except for populating null models, which your use of full river seg list fixes. My quick fix in the comment of the issue does "fix" it, but, my 2nd more in depth fix to insure a left outer join on the features in om_vahydro_metric_grid() fixes the problem correctly, has the speed, and keeps us at 1 data stream which is a better design goal imo.

Again, getting the fill list of river segments is the cause of the issue, not a bug in fn_extract_basin(). Let's give it a talk thru Monday @COBrogan @glenncampagna