D21750: HypothesisTest View and Dock

[Stefan Gerlach]

sgerlach added inline comments.

INLINE COMMENTS

> HypothesisTest.cpp:186
>      for (QString col : cols) {
>          if (col != "") {
>              column = dataSourceSpreadsheet->column(col);

QString::isEmpty()

> HypothesisTest.cpp:221
> +            if (n[i] < 1) {
> +                msg_box->setText(i18n("atleast one of selected column is empty"));
> +                msg_box->exec();

"At least"

> HypothesisTest.cpp:229
> +        findStatsCategorical(n, sum, mean, std, col1_name, col2_name);
> +        if (n[0] == -1) {
> +            msg_box->setText(i18n("Unequal size between %1 and %2", m_columns[0]->name(), m_columns[1]->name()));

readable error codes would be more convenient.

> HypothesisTest.cpp:244
> +
> +    QVariant row_major[] = {"", "N", "Sum", "Mean", "Std", col1_name, n[0], sum[0], mean[0], std[0], col2_name, n[1], sum[1], mean[1], std[1]};
> +    m_stats_table = getHtmlTable(3, 5, row_major);

please indent to make it easier to read.

> HypothesisTest.cpp:250
> +            m_currTestName = i18n("Two Sample Independent T Test for %1 vs %2", col1_name, col2_name);
> +            double t;
> +            int df;

"t_value" would be clearer

> HypothesisTest.cpp:255
> +
> +            resultModel->setRowCount(9);
> +            resultModel->setColumnCount(1);

why 9? please comment.

> HypothesisTest.cpp:280
> +
> +                    temp_msg = i18n("Null Hypothesis : Population mean of %1 %2 Population mean of %3", col1_name, QChar(0x2265), col2_name);
> +                    resultModel->setData(resultModel->index(0, 0), temp_msg, Qt::DisplayRole);

UTF8_QSTRING()?

> HypothesisTest.cpp:326
> +
> +            resultModel->setRowCount(8);
> +            resultModel->setColumnCount(1);

why 8?

> HypothesisTest.cpp:440
> +
> +    if (test == TestT) {
> +        m_currTestName = i18n("Two Sample Paired T Test for %1 vs %2", m_columns[0]->name(), m_columns[1]->name());

switch (test is an enum)

> HypothesisTest.cpp:442
> +        m_currTestName = i18n("Two Sample Paired T Test for %1 vs %2", m_columns[0]->name(), m_columns[1]->name());
> +        double t;
> +        int df;

t_value?

> HypothesisTest.cpp:608
>  
> -        if (n[i] < 1) {
> -            msg_box->setText(i18n("atleast one of selected column is empty"));
> -            msg_box->exec();
> -            return;
> +    if (test == TestT) {
> +        m_currTestName = i18n("One Sample T Test for %1", m_columns[0]->name());

better use "switch"?

> HypothesisTest.cpp:618
> +
> +        t = (mean - m_population_mean) / (std/qSqrt(n));
> +        df = n - 1;

double t = ...

> HypothesisTest.cpp:619
> +        t = (mean - m_population_mean) / (std/qSqrt(n));
> +        df = n - 1;
> +

double df = ..

> HypothesisTest.cpp:746
> +    count = qMin(count1, count2);
> +    double row1, row2;
> +    for (int i = 0; i < count; i++) {

cell1 and cell2 would be more specific.

> HypothesisTest.cpp:812
>  
> +void HypothesisTestPrivate::findStatsCategorical(int n[], double sum[], double mean[], double std[], QString &col1_name, QString &col2_name) {
> +

how about returning an error code instead of using n[0] for errors?

> HypothesisTest.cpp:860
> +            // this case occurs when there are more than two categorical variables in column 1
> +            // sending error code of -1;
> +            n[0] = -2;

not "-2"?

> HypothesisTest.cpp:866
> +
> +    if (col1_name == "" || col2_name == "") {
> +        n[0] = -2;

QString::isEmpty()

> HypothesisTest.cpp:880
> +
> +        if (name == col1_name) {
> +            std[0] += qPow( (value - mean[0]), 2);

you can omit braces in one-liners.

> HypothesisTest.cpp:900
> +
> +    QString table = "";
> +    table = "<style type=text/css>"

Maybe better define constant string in the header?

> HypothesisTestPrivate.h:81
> +
> +        void findStatsCategorical(int n[2], double sum[2], double mean[2], double std[2], QString &col1_name, QString &col2_name);
> +

order of parameter is different compared to other findStats-functions.

> HypothesisTestDock.cpp:87
> +    ui.pbPerformTest->setEnabled(false);
> +    ui.rb_h1_one_tail_2->setVisible(false);
> +    ui.rb_h1_one_tail_1->setVisible(false);

still not camelCase :-)

> HypothesisTestDock.cpp:517
> +    if (two_sample_paired) {
> +        ui.lCol2->setText("Independent Variable");
> +        return;

translation

> HypothesisTestDock.cpp:525
> +    if (col1->columnMode() == AbstractColumn::Integer || col1->columnMode() == AbstractColumn::Numeric) {
> +        ui.lCol2->setText("Independent Variable");
> +    }

translation

> HypothesisTestDock.cpp:528
> +    else {
> +        ui.lCol2->setText("Dependent Variable");
> +    }

translation

> HypothesisTestDock.h:72
>  //        bool fieldSelected(const QString&);
> +        bool ttest{false};
> +        bool ztest{false};

why not use a single variable of type testType?

> HypothesisTestDock.h:74
> +        bool ztest{false};
> +        bool two_sample_independent{false};
> +        bool two_sample_paired{false};

better use an enum for different kinds of test?

> HypothesisTestView.cpp:68
>      QFont font = m_testName->font();
>      font.setPointSize(15);
>      m_testName->setFont(font);

please define extra variable for font size

REPOSITORY
  R262 LabPlot

REVISION DETAIL
  https://phabricator.kde.org/D21750

To: devanshuagarwal, sgerlach, asemke
Cc: kde-edu, #labplot, narvaez, apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-edu/attachments/20190611/236c94b6/attachment-0001.html>