A few changes to properly pass storeResidual to BLAST #16
Conversation
| // do not accept negative levels, and only accept level 0 if it is the final level, | ||
| // as this is a proxy for storing the residual of a cascade | ||
| bool storeResidual = content->fStoreResidual || (tgt_number > 0 && cxlevels[tgt_number-1] == 0); | ||
| int tgt_checks = tgt_number - (storeResidual ? 1 : 0); |
There was a problem hiding this comment.
Humm that seems wrong. If 'content->fStoreResidual', the last element is probably not zero, unless we explicitly already added it (in which case the if can be simplified to tgt_number > 0 && cxlevels[tgt_number-1] == 0), isn't it?
There was a problem hiding this comment.
Thanks for double-checking me on this, Philippe.
I have observed that if content->fStoreResidua', then tgt_number, which is passed via precisionCascades.size() from TBasket, is 1 larger than content->fLen, so there is no point in examining cxlevels[tgt_number-1] in that case because the user never set that many levels. In other words, if I as a user provide 5 levels, and turn on fStoreResidual, then precisionCascades will have a size of 6 (it already has an extra buffer for the residual) and we only want to check the value of the 5 user-provided levels [0]...[4], not [5]. So I think this is the correct thing to do.
There was a problem hiding this comment.
but what if the user put both a zero at the end and has fStoreResidual == true ?
There was a problem hiding this comment.
Yeah, right now it is set to fail on that with the check on the level being less than 1. The code you've written will create two buffers and output files for the residual in that case, so if it is to be allowed, it should be caught further upstream of this to avoid creating two buffers/files for the residual.
There was a problem hiding this comment.
Maybe the simplest is to not support '0' as a compression level (and possible explicitly reject it in PrecisionCascadeCompressionConfig::PrecisionCascadeCompressionConfig in Compression.cxx. then the code above can be simplified, right?
There was a problem hiding this comment.
Perhaps this was the only controversial point in this PR?
There was a problem hiding this comment.
yes but since we migrate the i/o to store the 'storeResidual' at the end of the levels array (adding it based on the user input), it seems most of the PR is moot. Let me finish the change and see what I need to import.
| tgt[3] = (char)((out_sizes[tgt_idx] + 2) & 0xff); | ||
| tgt[4] = (char)((out_sizes[tgt_idx] >> 8) & 0xff); | ||
| tgt[5] = (char)((out_sizes[tgt_idx] >> 16) & 0xff); | ||
| size_t out_size = out_sizes[tgt_idx] + 2; |
There was a problem hiding this comment.
No. As you had written it, if out_sizes[tgt_idx] = 255, then tgt[3] = 1, tgt[4] = 0, and tgt[5] = 0, which would be interpreted as having a size of 1. The way I wrote it, tgt[4] would also become 1, so that the true size of 255 + 2 = 257 is preserved.
|
Most of the PR became unnecessary following the change of the I/O storage for the config. Change in out_size calculation was pushed manually. |
| auto content = reinterpret_cast<ROOT::Internal::PrecisionCascadeConfigArrayContent*>(configarray); | ||
| (void) configsize; |
There was a problem hiding this comment.
Since the assert is the only use of configsize and it is removed in Release build, this line prevent the compiler from complaining about unused variable/parameter.
While this fixes several little issues, it has not resolved problems I see with reading a precision cascade where the residual is stored. Still looking into that, but figured I'd go ahead and get these patches in.