-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Neural Snow code from paper revision #617
Conversation
pred_dswes[j - 1] = swepred | ||
nperiods = check_dates[j - 1] | ||
new_z = pred_zs[j - 1] + nperiods * Dates.value(Second(dt)) * zpred | ||
new_swe = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are new_swe and new_z used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below lines (326-327, 328-329) which set pred_zs[j] and pred_swes[j] to new_z and new_swe, with slight modification to handle gap traversal or specifically the two-model edge case having predictions which result in z <= SWE.
ext/neural_snow/ModelTools.jl
Outdated
sweinput[swe_sweidx] = pred_swes[j - 1] | ||
zpred = evaluate(zmodel, zinput)[1] | ||
swepred = evaluate(swemodel, sweinput)[1] | ||
pred_dzs[j - 1] = zpred |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be pred[j]? We used pred[j-1] above already to create zinput.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pred_dzs (the predicted dz values, aka the sequence of network outputs, the finite differences between the pred_zs vector) vector has a size one smaller than pred_zs (the predicted z values, the accumulated network outputs summed upon the initial condition), pred_dzs[i-1] references the step made from pred_zs[i-1] to pred_zs[i]. This syntax is also informed by iterating the loop from 2:end instead of 1:end-1.
ext/neural_snow/ModelTools.jl
Outdated
sweinput[swe_sweidx] = pred_swes[j - 1] | ||
zpred = evaluate(zmodel, zinput)[1] | ||
swepred = evaluate(swemodel, sweinput)[1] | ||
pred_dzs[j - 1] = zpred |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be helpful to use more descriptive/consistent names - is the output of evaluate a tendency/time derivative, or the next value? Here you say something like "dz = z"
pred_dzs[j - 1] = zpred
below you say new_z = pred_z + dt*zpred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left "zpred" as indicative of "the prediction made by the z-model as opposed to the swe-model", though I agree this could be ambiguous and instead interpreted as "prediction of depth z". If unit-based-names are more desired, the name would be "dzdt_pred" and pred_dzs would be better written as "pred_dzdts"
end | ||
|
||
""" | ||
serreze_qc(input, id, state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this specific to SNOTEL data? Also for yan_qc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serreze's and yan's filter is specific to SNOTEL sites/data.
""" | ||
livneh_bc(input) | ||
|
||
Apply undercatch bias control for water years in the manner outlined by Livneh (2014), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specific to SNOTEL data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method itself is not specific or designed to be specific to SNOTEL data, though in the paper in which it is created, it is only applied to SNOTEL data. The paper only leaves this note: "This simple method would not be appropriate for a maritime climate with greater occurrence of storms with mixed precipitation.", which does not apply to any of our data sources.
ext/neural_snow/DataTools.jl
Outdated
if sum(valid_idx) <= 2 | ||
continue | ||
end | ||
aprec = yeardata[valid_idx, :precip] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "aprec" mean as a variable name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"aprec" = "accumulated precipitation" values, as the raw snotel precipitation data is an accumulated precipitation (in the dataframe, accumulated precipitation is :precip, while the daily precipitation values, found via differencing in the accumulated precipitation, are :dprecipdt). Can change to "acc_prec" if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: add high-level direction in comments to point to bcqc_hourly/daily, or "look at these for scraping", so that people don't have to read the whole file for one intended usage. "accum" better than "acc". Make variables coherent for code understanding instead of using paper values, use comment to map code vars to paper vars
ext/neural_snow/DataTools.jl
Outdated
end | ||
aprec = yeardata[valid_idx, :precip] | ||
swe = yeardata[valid_idx, :SWE] | ||
precs = aprec[2:end] .- aprec[1:(end - 1)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here - precs is a different in aprec, cswe is a different in swe. it would be better if we oculd use a consistent and clearer naming for variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can go and change some names. This refers to taking the accumulated precipitation raw values and turning them into precipitation increment values (not "dprecipdt" yet because there is no division by dt, even though the value of dt in this case is 1 so the magnitudes are equivalent).
ext/neural_snow/DataTools.jl
Outdated
precs = aprec[2:end] .- aprec[1:(end - 1)] | ||
cswes = swe[2:end] .- swe[1:(end - 1)] | ||
|
||
sidx = -6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"cswe" = "change in swe" (I didn't want to use "dswe" due to the choice of using "dswes" below)
"sidx" = "start_index" and "eidx" = "end_index", which I use in a few different functions in the paper. I am open to different naming conventions/suggestions for these. The amount of abbreviation was for faster code prototyping and ingestability during development.
Some of the other names like p7n and whatnot are pulling formulaic names from the paper where they present their procedure, in order to aid transferability from verbal description to code.
sflags[s1] .= true | ||
end | ||
return sflags, hflags, wflags | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which of these filters did you end up using in the paper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are referring to "which of the three" between sflags, hflags, and wflags, I use all three (these are filters for the respective data columns, not the entire dataframe at once), see their application in bcqc_hourly(). If you are asking which of the site-level filters are utilized, all listed site filters are applied to the paper data.
tutorials look really nice! |
…from tutorial and test code and replaced with read-in file
Purpose
Update all files relating to neural snow depth parameterization code in accordance with recent paper revisions. This includes the Tools modules as well as the tutorials, as well as the associated tests and documentation.
To-do
Content