search

ceff-tech / ffc_api_client

An R client for the online Functional Flows Calculator API
https://ceff-tech.github.io/ffc_api_client
9 stars 3 forks source link

Error message and sanity checking improvements #59

Open nickrsan opened 4 years ago

nickrsan commented 4 years ago

@alyssaobester 1:15 PM the package is working for me on some gages and not others. any ideas as to why?

Nick Santos 2:47 can you send me some of the working and some of the not working gage ids?

Alyssa Obester 2:49 PM okay, thanks 2:51 this seem to be working: USGS gage: 11445500, SF American River near Lotus COMID: 14982092 and if I manually provide flow data for a COMID on the Middle Fork American River (COMID 14994421) it runs. (edited) 2:52 but this gage on San Antonio Creek doesn't work (11117500) and I get the following error:

Error in if (median >= predictions[["p10"]] && median <= predictions[["p90"]]) { : 
  missing value where TRUE/FALSE needed

2:53 And when I download that USGS gage data and try and feed it into the package, this is the error I get: 2:53

 (Error in process_data(flows_df, params, flow_field = flow_field, date_field = date_field,  : Bad Request (HTTP 400).

2:53 This actually was something Bronwen ran into, and I also tried it and got the same error 2:56 we also found a gage on USGS that doesn't seem to exist to the package, but that's a separate and less important issue right now

Nick Santos 3:15 PM is the gage new, or possibly lacking daily flow data? 3:16 the package keeps a list of info about the daily flow gages in CA - Ryan and I discussed missing gages as a possible hazard when he wrote that part of the gage code, but decided it was probably better than the alternative for now, I think 3:17 it'd be good for us to maybe provide a warning message if someone plugs in a nonexistent gage though

Alyssa Obester 3:17 PM I don't think so, this is the gage: https://waterdata.usgs.gov/ca/nwis/inventory/?site_no=11118501 3:17 I think a warning would be helpful

Nick Santos 3:17 PM that says something like "can't find gage - if you're sure it's in California and provides daily flow data, then this package may need an update - please file an issue at ____"

Alyssa Obester 3:17 PM yes yes

Nick Santos 3:18 PM we may be able to code around it and use the package internal data as a starting point and then try the internet services if it can't find it internally, but most of that probably isn't high on the todo list - and adds complexity - might just be worth always going to the internet, even if it's a bit slower 3:19 other failures will also need error messages, such as the san antonio creek gage - I think that's a data availability problem - probably need to do some sanity checks on the percentile data and make sure to only run appropriate alteration comparisons 3:19 huh, yeah, that is definitively not a new gage

Alyssa Obester 3:20 PM oh interesting

Nick Santos 3:21 PM we've had issues with gages with low or intermittent data - if the data was spotty, our comparisons ran into issues - we fixed at least one issue like that, which caused processing to fail, but probably need to figure out what's required to make san antonio creek work, and then have a test for it 3:21 OK, I'm going to file an issue with this conversation as the body of it

Alyssa Obester 3:21 PM there's also another gage that's right on top of where that gage should be (11118501) 3:21 but apparently it's not the same one

Nick Santos 3:21 PM huh - that's odd 3:22 did the one on top of it replace it?

Alyssa Obester 3:22 PM oops I meant to paste 11118500

Nick Santos 3:23 PM huh, it's older and lasts longer for daily discharge

Alyssa Obester 3:23 PM I don't think it replaced it

nickrsan commented 4 years ago

Test these gages:

gage,River
11118501,Ventura
11118500,Ventura
11117500,San Antonio
11468900,Mattole
11469000,Mattole
11476600,Bull Creek
11476500,SF Eel
11476000,SF Eel
11475800,SF Eel
11475500,SF Eel
11118000,Coyote
11116000,NF Matilija
11115500,Matilija
11258000,Fresno
11257500,Fresno
11465500,Mark West
kristaniguchi commented 3 years ago

Hi, did anyone resolve this issue: Error in if (median >= predictions[["p10"]] && median <= predictions[["p90"]]) { : missing value where TRUE/FALSE needed

I'm getting this error for multiple modeled timeseries and am working in a pretty flashy regime.

nickrsan commented 3 years ago

Hi Kris,

Nobody has resolved this as far as I know, but it does seem like it should be a high priority fix since it's going to impact usage of lots of gages in SoCal, I think. I'm going to listen in one the call today as well, in case discussion talks about the package at all.

kristaniguchi commented 3 years ago

Hi Nick,

We figured out that this was from having NULL values in the ref p10 percentiles for dry season baseflow in rivers.codefornature.org. Kirk is going to update the ref FFM predictions database from his end and replace NULL with zero. Apparently some of Ted’s model predictions were negative, and when they tried to round to 0 it gave them NULL in some instances. This should be a quick fix.

Thanks, Kris

-- Kris Taniguchi-Quan, PhD Scientist Southern California Coastal Water Research Project 3535 Harbor Blvd, Suite 110https://maps.google.com/?q=3535+Harbor+Blvd,+Suite+110+%0D%0A+Costa+Mesa,+CA+92626+%0D%0A+Office:+(714&entry=gmail&source=g Costa Mesa, CA 92626https://maps.google.com/?q=3535+Harbor+Blvd,+Suite+110+%0D%0A+Costa+Mesa,+CA+92626+%0D%0A+Office:+(714&entry=gmail&source=g Office: 714https://maps.google.com/?q=3535+Harbor+Blvd,+Suite+110+%0D%0A+Costa+Mesa,+CA+92626+%0D%0A+Office:+(714&entry=gmail&source=g-755-3221 Cell: 310-938-8814 kristinetq@sccwrp.orgmailto:kristinetq@sccwrp.org www.sccwrp.orghttp://www.sccwrp.org/

From: Nick Santos notifications@github.com Sent: Tuesday, October 13, 2020 1:03 PM To: ceff-tech/ffc_api_client ffc_api_client@noreply.github.com Cc: Kris Taniguchi-Quan kristinetq@sccwrp.org; Comment comment@noreply.github.com Subject: Re: [ceff-tech/ffc_api_client] Error message and sanity checking improvements (#59)

Hi Kris,

Nobody has resolved this as far as I know, but it does seem like it should be a high priority fix since it's going to impact usage of lots of gages in SoCal, I think. I'm going to listen in one the call today as well, in case discussion talks about the package at all.

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fceff-tech%2Fffc_api_client%2Fissues%2F59%23issuecomment-707976030&data=01%7C01%7Ckristinetq%40sccwrp.org%7Cfb901dd63f914d0f875c08d86fb2f307%7Ca4a8f23d1ae14b1c9902eaa153028190%7C0&sdata=pQ6%2FIjEwNDbZXTZ9%2Bn0Sn7Ghfpo%2FiniW0WhZ7rNSiQw%3D&reserved=0, or unsubscribehttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACTCCYEGIHY4MHUOYSIELXDSKSW6HANCNFSM4M4QJ75A&data=01%7C01%7Ckristinetq%40sccwrp.org%7Cfb901dd63f914d0f875c08d86fb2f307%7Ca4a8f23d1ae14b1c9902eaa153028190%7C0&sdata=C7jT7SeW0yclY%2FTFgw79ZoV%2BSKQaEGNT9Q6TZ3BEOLQ%3D&reserved=0.

nickrsan commented 3 years ago

OK, thanks for the update - that's good to know and makes sense. I'll still leave this open so we can add some tests in this package that ensure that code is correctly run for these gages that can flag any similar problems in the future if something changes. Thanks!

kristaniguchi commented 3 years ago

Sounds good! Thanks, Nick.

Kris

-- Kris Taniguchi-Quan, PhD Scientist Southern California Coastal Water Research Project 3535 Harbor Blvd, Suite 110https://maps.google.com/?q=3535+Harbor+Blvd,+Suite+110+%0D%0A+Costa+Mesa,+CA+92626+%0D%0A+Office:+(714&entry=gmail&source=g Costa Mesa, CA 92626https://maps.google.com/?q=3535+Harbor+Blvd,+Suite+110+%0D%0A+Costa+Mesa,+CA+92626+%0D%0A+Office:+(714&entry=gmail&source=g Office: 714https://maps.google.com/?q=3535+Harbor+Blvd,+Suite+110+%0D%0A+Costa+Mesa,+CA+92626+%0D%0A+Office:+(714&entry=gmail&source=g-755-3221 Cell: 310-938-8814 kristinetq@sccwrp.orgmailto:kristinetq@sccwrp.org www.sccwrp.orghttp://www.sccwrp.org/

From: Nick Santos notifications@github.com Sent: Tuesday, October 13, 2020 1:17 PM To: ceff-tech/ffc_api_client ffc_api_client@noreply.github.com Cc: Kris Taniguchi-Quan kristinetq@sccwrp.org; Comment comment@noreply.github.com Subject: Re: [ceff-tech/ffc_api_client] Error message and sanity checking improvements (#59)

OK, thanks for the update - that's good to know and makes sense. I'll still leave this open so we can add some tests in this package that ensure that code is correctly run for these gages that can flag any similar problems in the future if something changes. Thanks!

— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fceff-tech%2Fffc_api_client%2Fissues%2F59%23issuecomment-707983647&data=01%7C01%7Ckristinetq%40sccwrp.org%7Cd07b6c0bbb9648360a3c08d86fb50144%7Ca4a8f23d1ae14b1c9902eaa153028190%7C0&sdata=ftFMhKs58LFls8zdg0qx32KqGBk7C04TNuDTmxsdEi4%3D&reserved=0, or unsubscribehttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FACTCCYANFPIIZH53ZC47HMLSKSYVNANCNFSM4M4QJ75A&data=01%7C01%7Ckristinetq%40sccwrp.org%7Cd07b6c0bbb9648360a3c08d86fb50144%7Ca4a8f23d1ae14b1c9902eaa153028190%7C0&sdata=IJZ0TZk%2ByEoe71wdfV2tshTePeAlRPRyNBZSactuMtc%3D&reserved=0.