Conversation
|
|
||
|
|
||
| def test_zonal_mean_time_coarsening_25_steps_per_window(): | ||
| """Regression: forward_steps_in_memory=25 with factor=2 causes 13 vs 12 |
There was a problem hiding this comment.
| """Regression: forward_steps_in_memory=25 with factor=2 causes 13 vs 12 | |
| """ | |
| Regression: forward_steps_in_memory=25 with factor=2 causes 13 vs 12 |
| # if we have a buffer that means we didnt record the last batch | ||
| if self._buffer_gen: | ||
| start_idx = self.last_step | ||
| buffer_size = ( |
There was a problem hiding this comment.
I can't confidently review this file, the code is hard for me to understand. The for-if statements are nested up to 4 levels, and there's communication I don't understand happening between the first outer loop (for target_data) and the second outer loop (for gen_data). I also don't really know what "buffer" means in this context.
We chatted on Slack about potentially refactoring this to a helper function and cleaning it up before merging this, since this isn't a bug that usually affects us.
There was a problem hiding this comment.
It seems like the buffer is some residual that gets patched on to the start of the next coarsen? If so, a model like
def coarsen(data: T1, prefix_data: T2 | None) -> tuple[T1, T2 | None]:
for some suitable types T1 and T2 would be a good pattern to use
2 participants
Some of the underlying assumptions (to significantly decrease the logic needed for the buffers) made in the coarsening of zonal mean plots lead to an edge case where if the coarsening factor and windowing (e.g., number of forward steps in memory) were specific values we ended up having a size mismatch. To fix this more concrete logic is added to the buffer mechanism and added regression test to make more robust checks (if a batch needed to be skipped and put entirely into the buffer as well as the values that were found in #860)
Changes:
refactor the buffer mechanism in zonal mean time coarsener to allow for more robust logic including the potential for full batches to be put into the buffer
Tests added
Resolves #860