ED2: Minor revisions and additions #2203
Conversation
Add ability to create barebones ED2IN (no comments, just tags. Adding comments in the right positions can be buggy). Remove duplicated metstart calculation, and WRONG attempt to set it to MET_START tag (which doesn't actually exist).
| ed2in.text <- modify_ed2in( | ||
| ed2in.text, | ||
| latitude = as.numeric(settings$run$site$lat), | ||
| longitude = as.numeric(settings$run$site$lon), | ||
| met_driver = settings$run$inputs$met$path, | ||
| start_date = startdate, | ||
| end_date = enddate, | ||
| MET_START = metstart, | ||
| MET_END = metend, |
There was a problem hiding this comment.
Not clear why met start and end were removed from the code
There was a problem hiding this comment.
The way it was set here was wrong -- it would try to set the nonexistent tags MET_START and MET_END, rather than filling in templates like we used to do. This bug was cancelled out by another bug in write_ed2in that prevented it from writing non-existing ED2IN tags, but barebones=true in this PR resolved that, which is how I discovered this.
All that said, I saw from your Slack conversation with @bcow that people are actually using this tag, so I added it back (with, IMHO, more robust code) in 8481e73.
There was a problem hiding this comment.
See code starting here: https://github.com/PecanProject/pecan/pull/2203/files#diff-304b6e96334a5a21db89565447d07a5fR173
There was a problem hiding this comment.
I'm not sure what you mean by "it would try to set the nonexistent tags MET_START and MET_END". Those tags are in every versions of the ED2IN template I checked, so I'm still not following why we need the more complicated proc_met_startend function.
As a minor side note, I'm not sure why that function throws a warning for met years earlier that 1000, given that @crollinson @istfer @araiho are routinely running PalEON meteorology that starts earlier than that.
Also, I'm not sure why
settings[[c("run", "site", "met.start")]]
was deemed to be better than
settings$run$site$met.start
There was a problem hiding this comment.
Those tags are in every versions of the ED2IN template I checked
We haven't used the old mechanism for setting ED2IN tags based on gsub of things like @MET_START@ since March (#1870). The current mechanism sets ED2IN tags based on the name of the corresponding directive in the ED2IN, and there are no directives called MET_START or MET_END (the corresponding directives are METCYC1 and METCYCF). So the removed code referenced here would try to create namelist tags called NL%MET_START and NL%MET_END, which ED does not expect and would cause to crash on startup. However, this bug was cancelled out by another, where non-existent ED2IN tags were not actually being written to the ED2IN, which is why this error went undetected for a while.
proc_met_startend does effectively the same thing as the old one, but more verbosely and with some additional error checking. The old code did no validation at all of met.start/end being valid values -- it would happily accept arbitrary strings or any other nonsense that would be written to ED2IN, without any warning, which would cause ED to crash on initialization.
years earlier that 1000
The warning pre-year 1000 doesn't actually do anything except mention that the year looks suspicious. I thought this was fair since at least 90% of PEcAn runs are done after 1900. The goal here was to provide a bit more of a warning in case the tag was mis-interpreted or there was some error in the conversion from a date string to a year. In my opinion, it doesn't actively prevent anyone from doing paleo runs, and could help diagnose other potential problems, but if you disagree, I'm happy to remove it.
Why do I prefer [[ over $?
I don't like $ because, by default, it performs partial matching. So, for example, if we don't have a site tag but have a tag like site_soil or something similar, the current code will silently match that instead. Example:
l <- list(apple = list(red = 5))
l$a$r
# [1] 5That can introduce all kinds of problems that are very difficult to diagnose (and in fact, I have seen places elsewhere in the PEcAn code where this has caused unexpected behavior). OTOH, using [[ subsetting forces an exact match, which I find to be safer. If the recursive subsetting is confusing, I can change to settings[["run"]][["site"]][["met.start"]].
There was a problem hiding this comment.
re: pre-1000 A.D.: Unless somebody has found a workaround recently, ED didn't like 3-digit years when we did the PalEON long runs. We worked around this by just adding 1000 to the year (so 850 = 1850; 1850 - 2850).
There was a problem hiding this comment.
@ashiklom do you mean to say that in March you deprecated the use of @tag@ syntax in the ED2IN altogether??? I don't think that's common knowledge, since it wasn't explicitly discussed in that PR and my memory was that your PRs from around then were about facilitating your RTM runs (I wasn't aware of impacts on the main workflow). AFAIK everyone else has continued to use those tags without realizing they were just wasting their time.
Point taken on partial matching
For the dates, you don't have to drop the warning, but if you moved it to before 850AD you'd move from 90% of the runs to 99.99%
@crollinson I think I ran into similar issues with PEcAn and JULES that I thought I'd fixed by padding with leading zeros (e.g. 850 had to be 0850 otherwise the filename wasn't parsed correctly). Would be good to know if that makes ED happy too or if there's a deeper issue.
There was a problem hiding this comment.
you deprecated the use of @tag@ syntax in the ED2IN altogether?
Yes. See changes starting around here.
I don't think that's common knowledge
Happy to take the blame for that one. I didn't think people were creating new template ED2IN files, so I assumed as long as the resulting ED2IN was the same, the change wouldn't negatively affect anyone. To be fair, I did request reviews from you, @serbinsh , and @istfer , who I thought were the three most active ED users, for exactly this reason, and the CHANGELOG entry does mention that existing PEcAn code has been altered to use the new utilities I created, but I readily admit that both the PR message and that text are vague given the magnitude of the change.
For the dates, you don't have to drop the warning, but if you moved it to before 850AD you'd move from 90% of the runs to 99.99%
I suppose, but given the potential issues with 3-digit years pointed out by @crollinson, I'm tempted to leave this as is for now, since it provides an additional hint that such years can be problematic (at least until we can definitively sort out that pre-1000 year work without additional modification in the workflow).
ashiklom
added
Status: Ready
Description
Fix processing of
ed2in_tagsfrom XML. Now numbers (e.g.<TRAIT_PLASTICITY_SCHEME>0</TRAIT_PLASTICITY_SCHEME>) and numeric vectors (e.g.<INCLUDE_THESE_PFT>9,10,11,12</INCLUDE_THESE_PFT>) are correctly written to ED2IN without quotes.Add new
modeltag<all_pfts>. If "false" (default), set ED2IN'sINCLUDE_THESE_PFTto only PFTs explicitly configured through PEcAn. If "true", use all 17 of ED2's PFTs.Add new
modeltag<barebones_ed2in>. If "true", only write ED2IN tags, and do not include comment annotations. If "false" (default), try to transfer comments from ED2IN template to target ED2IN. Because of comments are written by matching line numbers, leaving this as "false" can lead to unexpected results whenever<ed2in_tags>contains tags missing from theED2INtemplate.Add some additional documentation for ED2
pecan.xmltags.Motivation and Context
Makes it easier to customize ED runs via the PEcAn XML, and fixes some minor bugs.
Review Time Estimate
Types of changes
Checklist: