USGS-R / river-dl

Deep learning model for predicting environmental variables on river systems
Creative Commons Zero v1.0 Universal
21 stars 15 forks source link

Offset correction #133

Closed janetrbarclay closed 2 years ago

janetrbarclay commented 2 years ago

Because the batch offset is 1 for trn (the default) and 0.5 for tst / val (specified), it seems the keep fraction should be 1 for trn and 0.5 for tst / val, yes?

https://github.com/USGS-R/river-dl/blob/714faa6f21170aa8b13f5970d3892e66fbbb2160/river_dl/preproc_utils.py#L381-L387

https://github.com/USGS-R/river-dl/blob/714faa6f21170aa8b13f5970d3892e66fbbb2160/river_dl/preproc_utils.py#L724-L740

SimonTopp commented 2 years ago

Janet and I talked about this offline a bit, TLDR: We're going to change it so there's a train_offset and test/val offset argument in the config file that will then propagate throughout the pipeline. Conversation rundown that led to this is below:

s: Ok, so, here's what I think happened with the offset stuff...... I think originally, Xiaowei was running the model with 0.5 offset for training and no offset for val/test and that this code never got updated when Jeff flipped those two. Xiaowei's way makes sense because latter half of the sequence predictions are generally better than the early ones, so if you have two predictions for each ob in the training it makes sense to only take the second one. The question is, do we update the fraction to match the default runs, or do we change the default runs to match the fraction

j: yes, that makes sense. Either way they need to match. The keep fraction that I've adjusted applies after the model is trained. Is there a way to adjust it in the training?

s: I agree they have to match for sure, just not sure what we should have the default be or if there was more logic to Jeff's choices that we're not seeing. Maybe ideally we'd add args to the config for offset train and offset test/val and those would propagate throughout the entire run

j: that sounds awesome. I'm happy to write that up. Would you be up for pasting the relevant parts of this chat into the discussion for the PR that I just opened?

::high fives::

aappling-usgs commented 2 years ago

I'm still struggling with the idea that we might want to treat train and val/test differently with respect to offsets. Why not use the same frac both places?

And if not the same frac both places, is there any possibility that a model trained to always start in January would struggle to make predictions when starting in July?

janetrbarclay commented 2 years ago

That's a good question and one I don't know the answer to. It seems clear to me that the batch offset and the keep fraction for predictions should be the same, but don't know what we gain / lose by having them different between the training and testing / validation partitions.

SimonTopp commented 2 years ago

I was chatting with @jzwart about the different start dates for train/test. He can probably elaborate, but my understanding from our conversation was that with the forecasting work it wasn't super important if the test sequence started Oct 1 (water year Jan 1) or not and that that was true regardless of if all the training sequences started Oct 1.

SimonTopp commented 2 years ago

Also, @aappling-usgs, I don't see any reason not to have them all same though, it would mean if we're using an offset we'd basically be doing the same thing I'm doing with GWN and be giving the model some look-back period that we drop

jzwart commented 2 years ago

Sorry I'm late to this conversation, but I agree that the changes look good. For the forecasting work, it was always important to start the test sequence immediately following the training sequence with the ending states of the training sequence if we wanted good predictions in the beginning of the test sequence (which was a priority). We didn't test for any increase in performance later in the test sequence in part because we didn't have a long enough test sequence (although we might now). We also didn't try stopping the training at different times of the year to see if that made a difference on model performance, but that could be an exploratory research avenue this year.

janetrbarclay commented 2 years ago

I'm a little bit confused - when you say that the test sequence started immediately after the training sequence, do you mean the overall testing dates or something related to the batches (like did you need an offset of 1)?


Janet Barclay U.S. Geological Survey New England Water Science Center Connecticut Office 101 Pitkin St. East Hartford, CT 06108

Phone (office) 860 291-6763 Fax 860 291-6799 Email @.**@*.**@*.***> https://www.usgs.gov/staff-profiles/janet-barclay


From: Jake Zwart @.> Sent: Wednesday, October 13, 2021 4:02 PM To: USGS-R/river-dl @.> Cc: Barclay, Janet R @.>; State change @.> Subject: [EXTERNAL] Re: [USGS-R/river-dl] Offset correction (#133)

This email has been received from outside of DOI - Use caution before clicking on links, opening attachments, or responding.

Sorry I'm late to this conversation, but I agree that the changes look good. For the forecasting work, it was always important to start the test sequence immediately following the training sequence with the ending states of the training sequence if we wanted good predictions in the beginning of the test sequence (which was a priority). We didn't test for any increase in performance later in the test sequence in part because we didn't have a long enough test sequence (although we might now). We also didn't try stopping the training at different times of the year to see if that made a difference on model performance, but that could be an exploratory research avenue this year.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHubhttps://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FUSGS-R%2Friver-dl%2Fpull%2F133%23issuecomment-942674834&data=04%7C01%7Cjbarclay%40usgs.gov%7C53f0b9ca101543a4106608d98e84764f%7C0693b5ba4b184d7b9341f32f400a5494%7C0%7C0%7C637697521843203178%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2BT5%2FYdz2SZVTejv3SuD8MJYNQ7OVk1%2BOokKZdhNr45E%3D&reserved=0, or unsubscribehttps://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAA5H7UE6CVS3W3EIGZM4XXTUGXQWFANCNFSM5FTPPLEQ&data=04%7C01%7Cjbarclay%40usgs.gov%7C53f0b9ca101543a4106608d98e84764f%7C0693b5ba4b184d7b9341f32f400a5494%7C0%7C0%7C637697521843203178%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nipTtXtkgL2MqFNt%2Fk2Hzoz2RD6qXsGlZ3O7LdD92Bc%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cjbarclay%40usgs.gov%7C53f0b9ca101543a4106608d98e84764f%7C0693b5ba4b184d7b9341f32f400a5494%7C0%7C0%7C637697521843213133%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=AboYQc3tyfyua42u5bhWjF7VwWQZ3i6MKSfOfZ8kufM%3D&reserved=0 or Androidhttps://gcc02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cjbarclay%40usgs.gov%7C53f0b9ca101543a4106608d98e84764f%7C0693b5ba4b184d7b9341f32f400a5494%7C0%7C0%7C637697521843213133%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=EKd6SnyiFLMGH1fkCoG63TE0aBGEavQ3%2Frwb0OvX0VU%3D&reserved=0.

jzwart commented 2 years ago

yes, sorry, I mean the overall testing dates started immediately after the training sequence. And there was no offset for the test sequences as each day we were making a prediction starting with the ending states from the previous day (with the first day's predictions starting the day after the last date of the training sequence)