search

BoulderCodeHub / Process-CRSS-Res

Repository for code used to process CRSS Results starting in April 2015
1 stars 3 forks source link

Generic dev pull #73

Closed cfelletter closed 5 years ago

cfelletter commented 5 years ago

@rabutler this is my new generic scripts I've designed to be modular with functions that will process both MTOM and CRSS results

rabutler-usbr commented 5 years ago

Nice, @cfelletter! This will be useful to use in all the comparisons we have to do. Also having generic.input.check() is smart.

I'll go ahead and merge into the dev since it doesn't affect any existing code, however I do have a few questions and suggestions.

  1. When processing multiple slots, as in Example 2, why don't you process the data all at once, rather than calling generic.scen.process() and RWDatatPlyr:::rw_scen_aggregate() multiple times? rw_scen_aggregate() will only read the rdf in once if there are multiple slots that all come from the same rdf file. This probably isn't that big of deal for MTOM or for the August CRSS, but it is when there are 35 different CRSS runs for one scenario it does end up mattering.

  2. Does this work with 35 different initial conditions that we need to put into one scenario "group" for comparison?

  3. It's best to stay away from a "." in function names. It gets a little confusing since R allows it, but functions that have a period are now generally supposed to be used for generics/methods.

  4. There are a lot of global variables being defined - specifically in generic.input.check() one gets reset in the function. Not sure if that particular update of the global variable is intended. Also, more generically global variables are not typically the preferred method for passing information; passing parameters is usually the way to go.

cfelletter commented 5 years ago

@rabutler-usbr thanks for the feedback. Maybe we can schedule a time for me to go over what I've done with you and @sbaker2423 before rolling this out to a bigger group. A number of those issues are things I knew but wasn't sure how to handle differently. Still I think this is a big step towards a shiny API for plotting. If a meeting sounds good feel free to look at my calendar and schedule something at your convince.

rabutler-usbr commented 5 years ago

I agree @cfelletter, great work to get closer to that Shiny tool!

We'll set something up for next week sometime.

rabutler-usbr commented 5 years ago

P.S. @cfelletter can we delete the Generic_Dev branch?