Info maps need "simulation_day" key for padding

[morrowcj]

After PR 2876 was merged, any output variables that might get reported via ReportGenerator are required to have a "simulation_day" key (indicating when the output was recorded) present in their info_map in order to be compatible with padding. Many output variables still do not meet this requirement.

Current Behavior

Because of the missing key, many report filters no longer work, producing errors like:

and

Expected Behavior

The following report filter (output/output_filters/report_all-variables.json) should create an output file with each output variable padded to all simulation days, without producing errors

{
  "multiple": [
    {
      "name": "All_Output",
      "filters": [
        ".*"
      ],
      "expand_data": true,
      "fill_value": ""
    }
  ]
}

Possible Solution

Add a reference to RufasTime.simulation_day to every output's info map, at the time it is recorded. Rather than manually editing each and every info map, it may be best to update OutputManager.add_variable and OutputManager.add_variable_bulk (and any other methods that add variables to the output pool) to update the variable info_map with this information automatically.

Context

Discussed in #2910

[morrowcj]

I tried adding a reference attribute to RufasTime within OutputManager.__init__ to facilitate methods automatically updating info maps with the current simulation_day, but doing so caused circular imports that I'm not knowledgeable enough to fix on my own (see simday-infomap branch, specifically commit f4bd190).

[ew3361zh]

Thanks for raising this issue Clay. A couple/few follow ups:

1. The reason for the circular import error when adding RufasTime to OutputManager is because of reporting the current day, simulated year, and calendar year of the simulation to OutputManager. Do we actually need to be reporting this? Does it influence other parts of the model/reporting? I don't recall why we do it.
2. If we do need it reported, we could just report it directly from SimulationEngine rather than from RufasTime. That single record_time() method is the only reason OutputManager is in RufasTime.
3. I think adding the simulation day from the instance of RufasTime that is an attribute of OutputManager could lead to unforeseen consequences and I'd only want to do it if it were very well tested. For example, one possible area of issue would be with OutputManager's add_variable_bulk().

[morrowcj]

Thanks for these excellent points, @ew3361zh

1. I'm not sure why those variables are reported to output manager, but I think it is good that they are if only because in a results file containing rows of data for each simulation day, having (the option of) columns that tell you the year/day of that simulation day is essential for using those data externally.

2. If we do need it reported, we could just report it directly from SimulationEngine rather than from RufasTime. That single record_time() method is the only reason OutputManager is in RufasTime.>

This makes good sense to me. I'll try moving it.

3. I think adding the simulation day from the instance of RufasTime that is an attribute of OutputManager could lead to unforeseen consequences and I'd only want to do it if it were very well tested. For example, one possible area of issue would be with OutputManager's add_variable_bulk().

Yeah, this is totally fair and I'm definitely open to alternative solutions - maybe it really is going to require manually adding these keys. But I figured it was worth trying a "cleaner" solution. I'll try your suggestion above and then do some cursory testing if it get it working.

[morrowcj]

After moving record_time to the SimulationEngine (2d6080f), there is still a circular import: