Differential D21977 Inline Comments

One Way Anova Test
Back to Revision

Inline Comments

DiffStatusReviewerCommentCreated
Currentsgerlach
please put comments describing the function before the function. Take a look at what doxygen expects. 
252 w
Currentsgerlach
do we have global variables? If yes, why do we need them? Avoiding global vars is a good programming practise 
252 w
Currentdevanshuagarwal
This function clears variables in HypothesisTestView, so that new results can be added. We dont actually have that many global variables.  
252 w
Currentsgerlach
Can you rename the function to "clearTestView" or something like this to make the purpose clearer? Are the global vars necessary? If not, please avoid them. 
252 w
Currentsgerlach
how efficient is qPow()? Does it make sense to use a gsl function for small inter powers to get better performance? See https://www.gnu.org/software/gsl/doc/html/math.html#small-integer-powers 
252 w
Currentdevanshuagarwal
Thanks :) 
252 w
Currentsgerlach
can you comment the variables? The names are not always self-explanatory (at least for me). 
252 w
Currentsgerlach
Can the following strings be made translatable? If yes, please do so.  
252 w
Currentsgerlach
this is only correct, when the distribution is symmetric? Is this true for the t-distribution? 
252 w
Currentdevanshuagarwal
Yes, firstly I used gsl_cdf_tdis_P(value, df) + gsl_cdf_tdis_P(-value, df). but the results from JASP and online calculator are not matching with this. The results are matching with 2*gsl_cdf_tdis_P(value, df) 
252 w
Currentsgerlach
OK. Please add a TODO-comment about this issue so we can check it later again. 
252 w
Currentsgerlach
not gsl_cdf_gaussian_P? 
252 w
Currentdevanshuagarwal
yes it should be gsl_cdf_gaussian_P. Actually, I have left ztest backend for now, I am not able to find online calculator and neither ztest is included in JASP. So for time being, I am not concentrating on it.  
252 w
Currentsgerlach
Please add a TODO-comment in the code that the value is not tested.  
252 w
Currentsgerlach
can we collect all the bool vars in an enum or a bit-field. Do we even need all of them (one_way=false is equal to two_way=true and vice-versa)? 
252 w
Currentdevanshuagarwal
No, technically we dont need separate variables, but it avoids confusion and also gives us the clear overview. I will think about using bit-field, but again, it will make things complex and will increase bugs.  
252 w
Currentsgerlach
If it would be more complex, leave it like it is. I just wanted to point out other popular solutions.  
252 w
Currentdevanshuagarwal
I too agree that the current setup is making code dirty. I think we should tinker on your idea of using bit-field. But, I am not getting the clear idea of how to proceed on it. Will each separate test will correspond to one bit?  
252 w
Currentsgerlach
if the following strings are user visible please add "i18n()". 
252 w
Currentsgerlach
please use cbTest->currentItem() and a matching enum (see above)  
252 w
Currentdevanshuagarwal
Is this not correct or less efficient?  
252 w
Currentsgerlach
It's surely less efficient since you call currentText() three times and compare each time to a string. Also the comparison will fail when the text is translated. 
252 w