Fix apply agglomerate #5499
Fix apply agglomerate #5499
Conversation
| val initialValues = cache(initialBoundingBox) | ||
| var range = initialValues.idRange | ||
| var currDimensions = initialValues.dimensions | ||
|
|
||
| var x = initialBoundingBox._1 | ||
| var y = initialBoundingBox._2 | ||
| var z = initialBoundingBox._3 + currDimensions._3 |
There was a problem hiding this comment.
@fm3 The error was caused by this premature advancement of z. Since we already read the range and currDimensions two lines above, this saved one iteration in the inner loop. However, this corrupted the values of nextBBinX and nextBBinY because they started at the wrong z value. This seems to never have been problem before, but it is conceptually wrong. Now we do one "unnecessary" inner iteration and cache lookup, but I think this is better than a complicated check whether we are in the first iteration. Let me know if you think the comments and this explanation are sufficient or if there still is something unclear.
There was a problem hiding this comment.
Cool, thanks, I think I get this a bit better now. Also thanks for the comments! They are very helpful!
If you agree, I think it would be great to also have a short higher-level comment block explaining what the cumsum.json is and what wk uses it for. Maybe even also for the agglomerate file itself. Feel free to merge after that :)
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Updated (unreleased) migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes