Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added the tests, estimates and made the general changes to One var Fit Model dialog #5861

Closed
wants to merge 179 commits into from

Conversation

Wycklife
Copy link
Contributor

Fixes #5810

@africanmathsinitiative/developers This is still in progress.

@shadrackkibet Is working on the general code structure for this dialog before I proceed with my work.

@shadrackkibet
Copy link
Collaborator

@Wycklife I am now taking up this to tidy the code and merge it into your branch. Thanks.

@rdstern
Copy link
Collaborator

rdstern commented Sep 9, 2020

@Wycklife I saw you added the separator - if so, that's great.

@Wycklife
Copy link
Contributor Author

Wycklife commented Sep 9, 2020

@Ivanluv @shadrackkibet and @rdstern Please could you review this?

@Ivanluv
Copy link
Contributor

Ivanluv commented Sep 10, 2020

The MKinfer and Trend packages seem not to available at first when testing and so they have to be installed first.Will this be a problem?


ucrChkConvertVariate.SetText("Convert to Numeric")
ucrChkConvertVariate.AddParameterValueFunctionNamesCondition(True, "data", "as.numeric", True)
ucrChkConvertVariate.AddParameterValueFunctionNamesCondition(False, "data", "as.numeric", False)
ucrChkConvertVariate.AddParameterValueFunctionNamesCondition(False, "data", frmMain.clsRLink.strInstatDataObject & "$get_columns_from_data", True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is being used

ucrDistribution = New ucrDistributions
ucrChkOmitMissing.Checked = True
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed

Comment on lines +639 to +641
Else
ucrDistributionChoice.SetExactDistributions()
End If
Copy link
Contributor

Choose a reason for hiding this comment

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

the else statement might not be needed.@shadrack could help us with this

Copy link
Contributor

@Patowhiz Patowhiz left a comment

Choose a reason for hiding this comment

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

Would recommend comments addition to explain certain parts of the code that are not obvious

Private Sub AddAsNumeric()
If rdoTest.Checked Then
If ucrChkConvertVariate.Checked Then
If ucrInputComboTests.GetText() = "sign" Then
Copy link
Contributor

Choose a reason for hiding this comment

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

@Wycklife I think code line 691 to 735 can be immensely be optimised and shortened by using Select Case conditional syntax

If rdoEstimate.Checked Then
If ucrChkConvertVariate.Checked Then
If ucrInputComboEstimate.GetText() = "mean" Then
clsMeanCIFunction.AddParameter("x", clsRFunctionParameter:=clsRConvertNumeric, iPosition:=0)
Copy link
Contributor

Choose a reason for hiding this comment

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

@Wycklife same here. The code block 798 to 815 can be optimised and shortened using the Select Case

Copy link
Contributor

Choose a reason for hiding this comment

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

and also code block 817 to 835

Private Sub ucrChkBinModify_ControlValueChanged(ucrChangedControl As ucrCore) Handles ucrChkBinModify.ControlValueChanged
SetBinomialTest()
BinomialConditions()
Private Sub tooltip()
Copy link
Contributor

Choose a reason for hiding this comment

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

@Wycklife probably use capital T, to be consistent with the other methods, Tooltip() instead of tooltip()


End Sub

Private Sub ToolTip1_Popup(sender As Object, e As PopupEventArgs) Handles tttests.Popup
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty sub Probably no longer needed?

Private clsRplotFunction, clsRplotPPComp, clsRplotCdfcomp, clsRplotQqComp, clsRplotDenscomp As New RFunction
Private clsBionomialFunction, clsProportionFunction, clsSignTestFunction, clsTtestFunction, clsWilcoxonFunction, clsZTestFunction, clsBartelFunction, clsBrFunction, clsRunsFunction, clsSenFunction, clsSerialCorrFunction, clsSnhFunction, clsAdFunction, clsCvmFunction, clsLillieFunction, clsPearsonFunction, clsSfFunction As New RFunction

Private Sub ToolTip2_Popup(sender As Object, e As PopupEventArgs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Empty sub, Probably no longer needed?

@Ivanluv
Copy link
Contributor

Ivanluv commented Sep 29, 2020

@Patowhiz am taking over this

@lloyddewit
Copy link
Contributor

This PR was replaced by PR #6004 and PR #6150

@lloyddewit lloyddewit closed this Mar 16, 2021
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.

Enable 2nd button in Model > Fit Model > One Variable
6 participants