Generalize creation of variables in models/ed/R/model2netcdf.ED2.R

[serbinsh]

trafficstars

Is your feature request related to a problem? Please describe. Based on this conversation: https://github.com/PecanProject/pecan/pull/2098#discussion_r216483153

Currently, both output conversion and ncvar definitions are numerically hard-coded in the function put_T_values and put_E_values. This works but uses "s" to offset the index by the length of existing variables between -T-, -E-, and -S- outputs. As pointed out by @ashiklom this could lead to later challenges when adding in more variables or if the order is changed. It would be preferable to access both by name from a list of vars or other.

Describe the solution you'd like https://github.com/PecanProject/pecan/pull/2098#discussion_r216483153

Access outputs and variables by name

Putting this here to open a discussion about this. @istfer is the best person to provide context for this before we make anymore changes.

[istfer]

we access standard netcdf file variables by name (the ones created by model2netcdf) if needed, iirc we don't assume a particular order anywhere after this. So, unless ED code changes (e.g. some vars in the middle are no longer outputted), adding more variables in model2netcdf with the number-wise approach should be trivial

but as I mentioned on the PR, I'm perfectly fine with extending nc_var list by name 👍

[serbinsh]

@istfer OK, that makes sense. I wrote this issue based on comments in the PR and not having a full appreciation of your refactor. Not really sure what is best here but I'm sure we can come to some compromise.

[github-actions[bot]]

This issue is stale because it has been open 365 days with no activity.

[infotroph]

@serbinsh @istfer is this issue still active?

[istfer]

I think we still have s and numbers in there. I'll defer to other people who are currently actively working with ED on whether to close or implement this

[Aariq]

I think I'm going to need to dig back into this function so I'll check this out. If I remember correctly, it's still quite fragile.

[Aariq]

I think part of the solution here is to use append() instead of assigning to specific indexes.

e.g. instead of

  nc_var[[s + 6]] <- ncdf4::ncvar_def("GPP", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999, 
                                    longname = "Gross Primary Productivity")

do:

  nc_var <- append(nc_var, ncdf4::ncvar_def("GPP", units = "kg C m-2 s-1", dim = list(lon, lat, t), missval = -999, 
                                    longname = "Gross Primary Productivity"))