fix(convertToQucsData): guard against out-of-bounds for extra_vars_dims#1550
Merged
Conversation
…nversion Use variable-specific lookups from `extra_vars_dims` instead of a sequential index counter, preventing `var_idx` out of bounds issues. Previously, one could achieve out-of-bounds issues if there was combination of `indep` and `extra_vars`.
This change uses the relative plot to save the correct nodes. We use 'setplot noiseX' and then use the relative wildcard save 'all' to get the equivalent of 'noiseX.all'. Previosuly, when using the fixed value of 'noise1.all' it would end up using the first result for every plot. Making it seems like there was one singular plot in the case of sweeps. Fixes: 09923a8 ("plot noise contributors")
|
Applied the patch to actual current branch will solve the issue 1526. |
Owner
|
Thanks for fix! Merged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This PR addresses an out-of-bounds issue originally reported in #1526, where one would crash if one did a parametric sweep with a fixed (scalar) variable.
The reason that it crashes is that we always prepend the
indepvariable to theextra_varslist, and in that TBbetais an additionalextra_var. When processingindepwe incrementvar_idx, and then when we processbetain this particular case, we try to access:extra_vars_dims(var_idx=1), however the size ofextra_vars_dimsis one less thanextra_vars, since we only track the size ofbeta, causing an out-of-bounds error. Since there was an explicit guard for!extra_vars_dims.empty(), one would not see this unless one had a TB withextra_vars.Changes
var_idxcorrectly, we just use a simple name lookup to get the correct dimension.noise1.allto the relativeall. This fixes the N-duplicate data one would get when doing a parametric sweep of N-steps.Example
Testbench taken from: #1526 (comment), modified to show that it works with PtsPerSummary as well.
TODOs
Note that when using
PtsPerSummaryfeature on noise sweep, one end up getting duplicates of the "dependent" signals, I assume that the data is the same, but it's duplicated, as seen here:The fix for this can probably be done outside this PR, since this fixes a crash.
Fixes: #1526