Skip to content

Comments

fix crash in View/Show statistics#288

Merged
gr5 merged 2 commits intomasterfrom
JST/fix283
Nov 28, 2025
Merged

fix crash in View/Show statistics#288
gr5 merged 2 commits intomasterfrom
JST/fix283

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented Nov 27, 2025

fix #283

@atsju atsju requested review from githubdoe and gr5 November 27, 2025 18:09
@github-actions
Copy link

🚀 New build available for commit 4a8d036
Download installer here

@github-actions
Copy link

🚀 New build available for commit 098dd33
Download installer here

@gr5
Copy link
Collaborator

gr5 commented Nov 28, 2025

This fix is fine and I tested it but these lines of code a little further down... YIKES.

DFTFringe/wftstats.cpp

Lines 286 to 302 in 098dd33

for (int j = 0; j < last; ++j){
int i = (ndx + j) % wavefronts.size();
//i = samndx[j];
cv::Mat resized = twaves[i]->workData.clone();
if (twaves[i]->workData.rows != rrows || twaves[i]->workData.cols != rcols){
cv::resize(twaves[i]->workData,resized, cv::Size(rcols, rrows));
}
sum += resized;
cv::Mat avg = sum/(j+1);
cv::Scalar mean,std;
cv::meanStdDev(resized,mean,std,mask);
double stdi = std.val[0]* md->lambda/outputLambda;
cv::meanStdDev(avg,mean,std,mask);
avgPoints << QPointF(j,std.val[0] * md->lambda/outputLambda);
wftPoints << QPointF(j,stdi);
trueNdx << i;
}

It appears that it is summing up all the wavefronts into a matrix called sum which is great but inside the loop it is calculating the average. So if you have 100 wavefronts (Yordan often has 100 wavefronts) and looking at the statistics it calculates the average 100 times (not 99, 100). And calculates the standard deviation 100 times. That code (lines 294-297) should be outside the for loop. I'm pretty sure.

Also one could just put only the zernike's in a table instead of creating an entire wavefront matrix and do the std deviation on just the table of zernikes (once for each of the 49 or so zernikes).

Anyway, I don't want to mess with it as it would need careful testing I think.

Copy link
Collaborator

@gr5 gr5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good looks great. Tested in qt5 and it works.

@atsju
Copy link
Collaborator Author

atsju commented Nov 28, 2025

This fix is fine and I tested it but these lines of code a little further down... YIKES.

DFTFringe/wftstats.cpp

Lines 286 to 302 in 098dd33

for (int j = 0; j < last; ++j){
int i = (ndx + j) % wavefronts.size();
//i = samndx[j];
cv::Mat resized = twaves[i]->workData.clone();
if (twaves[i]->workData.rows != rrows || twaves[i]->workData.cols != rcols){
cv::resize(twaves[i]->workData,resized, cv::Size(rcols, rrows));
}
sum += resized;
cv::Mat avg = sum/(j+1);
cv::Scalar mean,std;
cv::meanStdDev(resized,mean,std,mask);
double stdi = std.val[0]* md->lambda/outputLambda;
cv::meanStdDev(avg,mean,std,mask);
avgPoints << QPointF(j,std.val[0] * md->lambda/outputLambda);
wftPoints << QPointF(j,stdi);
trueNdx << i;
}

It appears that it is summing up all the wavefronts into a matrix called sum which is great but inside the loop it is calculating the average. So if you have 100 wavefronts (Yordan often has 100 wavefronts) and looking at the statistics it calculates the average 100 times (not 99, 100). And calculates the standard deviation 100 times. That code (lines 294-297) should be outside the for loop. I'm pretty sure.

Also one could just put only the zernike's in a table instead of creating an entire wavefront matrix and do the std deviation on just the table of zernikes (once for each of the 49 or so zernikes).

Anyway, I don't want to mess with it as it would need careful testing I think.

I already hesitated to mess too much with the part of code I touched.
There are many places that could be improved. I think we could review file by file... But it's lots of tests

@gr5 gr5 merged commit 934055d into master Nov 28, 2025
14 checks passed
@githubdoe
Copy link
Owner

The stats makes a running average. Showing the average as you add each wave front. The purpose is to show how the average converges to a stable average and when that happens / if that happens.

I don't know if that is what you are looking at but if so that is the purpose.

@atsju atsju deleted the JST/fix283 branch November 28, 2025 18:30
@gr5
Copy link
Collaborator

gr5 commented Nov 29, 2025

Yeah so it's good we didn't mess with that code. I thought maybe you put some lines of code in there by accident but then was thinking about the "avg=sum/j+1" part and the j+1 part makes me realize you meant it to be in the loop. So okay, that makes sense.

@githubdoe
Copy link
Owner

Just a reminder of the history of that feature. We knew that if you can make the noise random (air currents sort of are) then if you average enough wave fronts samples to randomize the noise you can remove the noise. The trick is knowing when you have enough samples. That depends on the number of samples and the noise. So I wanted a way to show when enough samples had been taken. That feature average each sample as a running average and when/if the STD or other values you watch settle into the lowest value you have enough samples.

@gr5
Copy link
Collaborator

gr5 commented Nov 29, 2025

Ah. That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8.2.0 crashes in View/Show statistics

3 participants