Adds code to ensure parameter array indices are valid#236
Adds code to ensure parameter array indices are valid#236DrPaulSharp merged 7 commits intoRascalSoftware:masterfrom
Conversation
| domainRatioArray, dataPresent, nParams, params, ~, resample,... | ||
| contrastBackgroundActions, cCustFiles, useImaginary] = extractProblemParams(problemStruct); | ||
|
|
||
| checkIndices(contrastBackgroundIndices, contrastQzshiftIndices,... |
There was a problem hiding this comment.
Is it possible to do this check earlier maybe in parseClassToStructs? This should reduce duplication and any chance of affecting performance. The function can take problemStruct as input also
There was a problem hiding this comment.
I was cautious owing to the fact that the reflectivity calculation is performed numerous times within the minimisers, with changes to problemStruct. Having looked through carefully though, it seems like the changes made in unpackParams cannot affect the length of the parameter arrays, so we should be ok. Let me know if you can think of anything that might be affected though.
API/projectClass/contrastsClass.m
Outdated
| contrastBulkIns = ones(1,nContrasts); | ||
| contrastBulkOuts = ones(1,nContrasts); | ||
| contrastDomainRatios = zeros(1,nContrasts); | ||
| contrastDomainRatios = ones(1,nContrasts); |
There was a problem hiding this comment.
I am not sure why this is changed?
There was a problem hiding this comment.
Just for consistency with the rest of the arrays and tests now that each entry is filled in or set to -1
There was a problem hiding this comment.
Just cautious we don't break something by changing the default. Is it possible one of the contrast does not have a domain ratio property but the value defaults to 1 now instead of the original 0?
There was a problem hiding this comment.
A few lines further down, we loop through each contrast and set the value if the contrast has a domain ratio, or set it to -1. I therefore can't see any issue with this change. Having said that, I take your point that there is potential for problems with this default, so I'll change back. I'm inclined to think, especially with the check indices routine now present, that setting all of these to a default of zero is the better option. One to talk over on Monday maybe?
2 participants
No description provided.