HARPgroup / model_meteorology

0 stars 0 forks source link

Workflow step `geo -> process -> 02_daily_data`: Rscripts to produce daily values for Precipitation and Flow at a given gage #63

Open mwdunlap2004 opened 1 month ago

mwdunlap2004 commented 1 month ago
rburghol commented 1 month ago

Hey @mwdunlap2004 -- I updated this to note that we want to join the flow data at this step in the workflow, also, since the precip data does not know the drainage area for the gage (we are including it in the usgs data download as a column), I think we may as well go ahead and do the area weighting to add a column for precip_cfs here.

I want to hear if you or @COBrogan has reasons not to -- like, it is combining function which we want to avoid :)

COBrogan commented 1 month ago

@rburghol I have no problem with this. I think this makes sense to me. This way the daily data is just fully assembled at this step, outputting a file that is ready for any further processing and just has ALL daily data that may be relevant to the analysis. It also appears I wrote some code that depended on this step happening anyways.... so I approve!

mwdunlap2004 commented 1 month ago

Are you saying we'd want it as a part of hydroimport? I feel like that might be a lot to include in one function, but we can definitely do it. I did also edit my usgsdata code to add the da column, all it is a column that just repeats the value for every day, but I don't think that will have any negative effects on our code I'll just have to change how we call it when we're doing the cfs.

mwdunlap2004 commented 1 month ago

Is the drainage area something we want to keep in our daily data? Or once we use it do we not need it?

rburghol commented 1 month ago

Cool @mwdunlap2004 - thanks for making that edit to the USGS code - we will want to migrate that code to the meta_model/scripts/river directory as soon as you are ready (or we can do it, but you may benefit from the PR process for that, and then you'll get credit for the code update in github).

I am with you both, @COBrogan we definitely MUST have the joining of flow and precip in a single file as we all agreed on, but I also am with Michael that it breaks our design rule of 1 step, 1 function. It also means that we add a 3rd argument to the script (the path to the USGS file). But, if we decided that we wanted to koin in the USGS data here anyhow (is that what we decided?), then, we should go ahead and do the precip_cfs conversion since all the data is already there and it is a simple mathematical operation.

To me, the alternative is to make 02_daily_data ONLY summarize precip at a daily time scale, then have a separate function like 03_join_precip_flow to add the obs_flow and precip_cfs columns.

mwdunlap2004 commented 1 month ago

I don't know how to add the update to the meta model so I would appreciate if you could do it (and maybe meet next week to talk about it). I'll probably push to harp archive tomorrow, once we've made a decision on how to handle the daily_data step. Also I just noticed that you mention that it includes precip_mm, right now I haven't been including that, I can definitely change the code to include it, especially if we only need it for the daily step, just let me know.

COBrogan commented 1 month ago

I don't have a strong opinion on this and I think you guys have a good point here. Thus far maintained one step = one function. Breaking it out further is fine by me. At the end of the day, we are striving to make our architecture as consistent as possible. To that end, a 03_join_precip_flow step would help out with that.

rburghol commented 1 month ago

Let's go ahead and create that separate step then, we can always merge the code if we're discontented :).

mwdunlap2004 commented 1 month ago

I've updated the hydroimport and compdata functions to do this, I also added the calculations for precip_cfs to comp data. As well as adding drainage area as a column to our flow data. If we ever change our minds, it's actually a really simple change, but I do think it makes sense to split them up.

rburghol commented 1 month ago

This is great @mwdunlap2004 thanks! I will wait till Monday to do the code merge into the meta_model repo, so we can review that process with you and let you do it (though I have included the steps to do this in the issue checklist in the body of the issue if you want to take a crack). This is really coming together :)

I created an issue for this #69