Automatically adds "simulation_day" to info_maps when added to OutputManager

[morrowcj]

Because simulation_day is required for data padding (#2867, #2921) in the ReportGenerator, automatically add simulation_day to every info_map when variables are added to the OutputManager (via add_variable() and add_variable_bulk()) from the main simulation loop.

Also allows for a manually provided time object to be used for recording simulation_day in this way, which allows
for variables added outside of the simulation loop (e.g, herd initialization).

Context

Closes #2919

What

Test plan

two tests were added to test this feature: test_add_variable_infomap_simulation_day and test_bulk_add_variable_infomap_simulation_day, which test that the info maps are updated properly within the OutputManager's pools.

The testing filter file that I was originally targeting that led to this PR was:

{
  "multiple": [
    {
      "name": "FieldEvaluation",
      "filters": [
        ".*RufasTime.*",
        ".*field=.*",
        ".*EmissionsEstimator.*"
      ],
      "expand_data": true
    }
  ]
}

In an ideal world, one would also be able to get all variables with padding (".*" filter), but this is complicated by the herd initialization whose variables are created before the main simulation loop and, therefore, will not be properly aligned with the other variables after padding operations. In this case, then, a perfect filter would exclude all such variables. Or perhaps the data padder could updated to exclude any variables without the simulation_day info map value even if those values are included in the filter (i.e., compatibility with .*).

Input Changes

No input changes.

Output Changes

Adds simulation_day to all info maps.

[github-actions]

Current Coverage: %

Mypy errors on add_infomap_simday branch: 1201
Mypy errors on dev branch: 1195
6 more errors on add_infomap_simday branch

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.
🚨 Flake8 linting errors were found. Please fix the linting issues.
🚨 Some tests have failed.

[github-actions]

Current Coverage: %

Mypy errors on add_infomap_simday branch: 1201
Mypy errors on dev branch: 1195
6 more errors on add_infomap_simday branch

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.
🚨 Flake8 linting errors were found. Please fix the linting issues.
🚨 Some tests have failed.

[github-actions]

Current Coverage: %

Mypy errors on add_infomap_simday branch: 1196
Mypy errors on dev branch: 1191
5 more errors on add_infomap_simday branch

[github-actions]

🚨 Please update the changelog. This PR cannot be merged until changelog.md is updated.
🚨 Flake8 linting errors were found. Please fix the linting issues.
🚨 Some tests have failed.

[morrowcj]

I can't figure out why my tests are failing. I'm getting an error that "'NoneType' object has no attribute 'with_segments'", but I can't figure out what is causing it.

pytest tests/test_output_manager.py

An example of one of the errors:

@matthew7838, any thoughts? This feels like an issue mocking the OutputManager, but maybe it is a problem with the actual code. 🤷‍♂️

[morrowcj]

I was able to fix it with 875ea68 by mocking om.chunkification=False, which indicates to me that there was some leakage from the previous tests (i.e., chunkificaiton tests).

Now it seems like my failures are actually due to faulty tests - so I'll work on fixing those next.

[github-actions]

Current Coverage: %

Mypy errors on add_infomap_simday branch: 1196
Mypy errors on dev branch: 1191
5 more errors on add_infomap_simday branch

[github-actions]

Current Coverage: 99%

Mypy errors on add_infomap_simday branch: 1196
Mypy errors on dev branch: 1191
5 more errors on add_infomap_simday branch

[github-actions]

🚨 Flake8 linting errors were found. Please fix the linting issues.

[github-actions]

Current Coverage: 99%

Mypy errors on add_infomap_simday branch: 1197
Mypy errors on dev branch: 1191
6 more errors on add_infomap_simday branch

[github-actions]

🚨 Flake8 linting errors were found. Please fix the linting issues.

[github-actions]

Current Coverage: 99%

Mypy errors on add_infomap_simday branch: 1197
Mypy errors on dev branch: 1191
6 more errors on add_infomap_simday branch

[github-actions]

🚨 Flake8 linting errors were found. Please fix the linting issues.

[ew3361zh]

I think this looks good so far, I would want to test it out with some example filters to see the effect in action and make sure we're tracking potential side-effects of mutating the info_map like this. Maybe consider a separate small helper function to specifically mutate the info map (or create a copy) to be as transparent as possible with what's happening here?

Also curious if this is working to generate the outputs you're looking for?

[ew3361zh]

I think it would be helpful in the test section of the PR writeup to have some specific filters with variables you're looking at so we can see the intended effect in action.

[morrowcj]

I've added this information to the write-up. The filter that I was targeting works now and the output is as expected, but it is probably too narrow for fully testing this functionality. So, I'm certainly open to some filter suggestions.

[morrowcj]

@ew3361zh,

Maybe consider a separate small helper function to specifically mutate the info map (or create a copy) to be as transparent as possible with what's happening here?

Could you elaborate on this a bit? I want to make sure I understand what you're going for.

[github-actions]

Current Coverage: 99%

Mypy errors on add_infomap_simday branch: 1197
Mypy errors on dev branch: 1191
6 more errors on add_infomap_simday branch

[ew3361zh]

@ew3361zh,

Maybe consider a separate small helper function to specifically mutate the info map (or create a copy) to be as transparent as possible with what's happening here?

Could you elaborate on this a bit? I want to make sure I understand what you're going for.

For sure! Right now, this version of add_variable() has a docstrings description of "Adds a variable to the pool." But it actually is doing more than a few things:

1. Validating that the info_map sent with the variable has an entry for units (and that the info_map has the proper number of units entries if the variable sent is a dict)
2. Checking if the info_map has a simulation_day entry.
3. Checking whether the simulation_day should be added to or overwritten in the info_map.
4. Adding it to the variable pool.
5. Adjusting the variable to the variable usage counter accordingly.
6. Checking if chunkification thresholds have been reached.

I think trying to simplify some of this where possible to keep this function focused on its main task would be helpful. We could do this by adding a function that takes one more more of these items and does them itself and returns the result to the add_variable() function.

With info_map sim_day stuff we could make a copy of the info_map and send that to a helper function to do what's a very simple task but which keeps the add_variable() function cleaner, easier to test, and more straightforward.

e.g.

def _handle_info_map_sim_day(
    self,
    info_map: dict[str, Any],
    overwrite_simulation_day: bool,
    time: RufasTime | None = None,
) -> dict[str, Any]:
    info_map_to_use = dict(info_map)
    time_to_use = time if time is not None else self.time

    should_set_simulation_day = overwrite_simulation_day or "simulation_day" not in info_map_to_use
    if time_to_use is not None and should_set_simulation_day:
        info_map_to_use["simulation_day"] = time_to_use.simulation_day

    return info_map_to_use