Remove conversion and checkTemp helpers per TODO

[Thomas-Sedhom]

Description

This PR removes the local conversion and checkTemp helper functions from models/ed/R/model2netcdf.ED2.R and replaces their usage with inline ifelse statements at the existing call sites.

Motivation and Context

Review Time Estimate

• Immediately
• Within one week
• When possible

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My change requires a change to the documentation.
• My name is in the list of CITATION.cff
• I agree that PEcAn Project may distribute my contribution under any or all of
  • the same license as the existing code,
  • and/or the BSD 3-clause license.
• I have updated the CHANGELOG.md.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.

[infotroph]

Thanks for opening this. See my inline comment for the reasons I don't think ifelse is the right direction.

As a more general point, it's worth opening an issue to discuss changes like this even if there's an existing TODO -- first because programmers (me included!) tend to write TODOs when they're in a hurry and chances are they intended to defer the "thinking about the design and considering other options" part as well as the implementation of the change, second because often the code changes around the comment and leaves it outdated, and third because we have so many TODOs in the codebase that it's worth checking on priorities before picking up any one of them. Personally I'd rate this one as low priority but worth finishing now that you've started on it.

[infotroph]

I didn't write this comment (that was probably @serbinsh years ago), but I recommend against it -- inline ifelse statements get messy fast and don't show intent as well as named helpers.

But while I'm looking at it, a higher-level fix that would eliminate the need for these operations would be to set all the missing values in out to literal NAs instead of -999. Then numeric conversions can be done on whole columns without worrying about disturbing the missing value flags, and the ncdf4 library will handle them automatically.

This suggestion does assume out is only written to netcdf and not passed to any other processes that need the missing values to be -999, so someone who knows the ED model should weigh in on whether that's true. Even if it does get passed elsewhere, it may still be cleaner overall to convert the NAs once at the end rather than work around them when converting each column.

[Thomas-Sedhom]

I took a closer look at the data flow in model2netcdf.ED2.R. From what I found, the out object built by read_T_files() / read_E_files() is mainly used as an intermediate step before writing to NetCDF, and I did not find any internal code path in this repo where it is passed to other ED processing that specifically depends on -999.
I also noticed that the E-file path already seems to rely on NA internally in some places and still writes NetCDF variables with missval = -999, so your suggestion looks reasonable to me. The main remaining concern seems to be whether changing read_T_files() output from -999 to NA would affect any external callers, since these helper functions are exported. If that concern is not important, I agree that using NA internally and only handling missing values at the NetCDF boundary would likely be cleaner.

[Thomas-Sedhom]

Thanks for opening this. See my inline comment for the reasons I don't think ifelse is the right direction.

As a more general point, it's worth opening an issue to discuss changes like this even if there's an existing TODO -- first because programmers (me included!) tend to write TODOs when they're in a hurry and chances are they intended to defer the "thinking about the design and considering other options" part as well as the implementation of the change, second because often the code changes around the comment and leaves it outdated, and third because we have so many TODOs in the codebase that it's worth checking on priorities before picking up any one of them. Personally I'd rate this one as low priority but worth finishing now that you've started on it.

Sorry about that, I wasn't aware of the convention around TODOs, I assumed the comment was enough to justify jumping in. Good to know for the future. Should I go ahead and open an issue for this so we can continue the discussion there?