-
Notifications
You must be signed in to change notification settings - Fork 234
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
Generalize creation of variables in models/ed/R/model2netcdf.ED2.R #2102
Comments
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 👍 |
@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. |
This issue is stale because it has been open 365 days with no activity. |
I think we still have |
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. |
I think part of the solution here is to use e.g. instead of
do:
|
Is your feature request related to a problem? Please describe.
Based on this conversation:
#2098 (comment)
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
#2098 (comment)
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.
The text was updated successfully, but these errors were encountered: