Unit testing tolerances + more consistent definition of SNR #79
Conversation
Dynamic scaling adjusted. Also boundaries adjusted Remove normalisation as signal isnormalized. Remove algorithm-specific tolerance boundaries such that default boundaries are used Take medians of parameters as means give some machine precission error...
oliverchampion
requested review from
etpeterson,
IvanARashid and
oscarjalnefjord
| 'D': np.mean(Dim[selector], axis=0), | ||
| 'f': np.mean(fim[selector], axis=0), | ||
| 'Dp': np.mean(Dpim[selector], axis=0), | ||
| 'D': np.median(Dim[selector], axis=0), |
There was a problem hiding this comment.
Yes, somehow there was a weird rounding happening in the np.mean that made values become 0.0000000000000001 off or so. So all round values become D= 0.0029999999999 instead of 0.003 etc.
| scale = dyn_rtol["offset"] + dyn_rtol["ratio"]*ratio + dyn_rtol["noise"]*noise + dyn_rtol["noiseCrossRatio"]*ratio*noise | ||
| tolerances["rtol"] = {"f": scale*dyn_rtol["f"], "D": scale*dyn_rtol["D"], "Dp": scale*dyn_rtol["Dp"]} | ||
| scale = dyn_rtol["offset"] + dyn_rtol["noise"]*data["noise"] | ||
| if dyn_rtol["scale_f"]: |
There was a problem hiding this comment.
I think you want to also check if this key exists.
| scale = dyn_atol["offset"] + dyn_atol["ratio"]*ratio + dyn_atol["noise"]*noise + dyn_atol["noiseCrossRatio"]*ratio*noise | ||
| tolerances["atol"] = {"f": scale*dyn_atol["f"], "D": scale*dyn_atol["D"], "Dp": scale*dyn_atol["Dp"]} | ||
| scale = dyn_atol["offset"] + dyn_atol["noise"]*data["noise"] | ||
| if dyn_atol["scale_f"]: |
| scale = dyn_rtol["offset"] + dyn_rtol["ratio"]*ratio + dyn_rtol["noise"]*noise + dyn_rtol["noiseCrossRatio"]*ratio*noise | ||
| tolerances["rtol"] = {"f": scale*dyn_rtol["f"], "D": scale*dyn_rtol["D"], "Dp": scale*dyn_rtol["Dp"]} | ||
| scale = dyn_rtol["offset"] + dyn_rtol["noise"]*data["noise"] | ||
| if dyn_rtol["scale_f"]: |
There was a problem hiding this comment.
I think this is a misnomer and should be renamed. I thought something called scale_f would be scaling the f parameter. Instead it scales the others by f.
| if dyn_rtol["scale_f"]: | ||
| tolerances["rtol"] = {"f": scale*dyn_rtol["f"], "D": scale*dyn_rtol["D"]/(1-data['f']+epsilon), "Dp": scale*dyn_rtol["Dp"]/(data['f']+epsilon)} | ||
| else: | ||
| tolerances["rtol"] = {"f": scale * dyn_rtol["f"], "D": scale * dyn_rtol["D"]/0.9,"Dp": scale * dyn_rtol["Dp"]/0.1} |
There was a problem hiding this comment.
Why 0.9 and 0.1? Also why would we not want to scale by f?
There was a problem hiding this comment.
Good point. I now automatically scale by f, instead. This solves this comment and the three above
| signal /= signal[0] | ||
| ratio = 1 / signal[0] | ||
| return signal, ratio | ||
| signal = np.abs(signal) #Oliver: do we need this for unit testing? It will bias the fits by forcing positive signals. |
There was a problem hiding this comment.
Probably not necessary.
There was a problem hiding this comment.
This had to be moved to some individual fit algorithms to prevent them from failing
| ratio = 1 / signal[0] | ||
| return signal, ratio | ||
| signal = np.abs(signal) #Oliver: do we need this for unit testing? It will bias the fits by forcing positive signals. | ||
| signal /= signal[0] # this scales noise differenly. I have now made sure signal is produced with amplitude 1 if right flags are selected. Then SNR is fixed. Then, this sentence may be redundant |
There was a problem hiding this comment.
Should probably be removed, at least eventually each algorithm should have their own scaling built in. For now maybe it needs to stay though?
There was a problem hiding this comment.
Needs to go to wrapper. For now I also rescale here. Can be removed once fixed in wrapper.
|
I think now it simplifies testing. Not sure whether "dynamic testing" has a place in our testing line, although it cannot harm to keep it in there:). |
Added testing for whether bounds are respected and whether timing of algorithms make sense
Also changed fit_result["D*"] back to fit_result["Dp"], as now both names were used throughout the scripts
Describe the changes you have made in this PR
1: I want to have a more consistent definition of SNR in the phantom. By putting T1T2 to False, T1 and T2 effects are ignored and all signal starts at 1. This means that SNR = 1/noise.
2: I have tried to make tolerances scale with SNR and f. --> This is also meant to get the discussion going for Friday and can be adapted accordingly
Link this PR to an issue [optional]
1 Fixes _#76
2 Fixes _#75
Checklist