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

Improvements to the Describe > Two variable Summarise Dialog #8582

Merged
merged 47 commits into from
Nov 3, 2023

Conversation

Vitalis95
Copy link
Contributor

Fixes #7262
Replace PR #7999
@rdstern have a look at the progress so far.
@N-thony @lilyclements
Thanks

Vitalis95 and others added 30 commits November 24, 2022 10:47
@rdstern
Copy link
Collaborator

rdstern commented Oct 19, 2023

@Vitalis95 this is now looking very promising. I am ignoring the correlations and ANOVA options for now.
Here is the first option:
image

It all seems to work, but we need to add 2 options.
a) Margin - as in the other option i.e. Factor by Factor
b) A display option - though I a not sure of the existing one in the general Summary tables - it doesn't seem ideal. I am not sure quite what we need here, so let me explain

Here is the current layout:

image

a) The alternative would be to have Summaries in Rows. Perhaps that's just a checkbox? If checked it swaps the rows and columns, in the above display.
b) This is giving all the summaries for one variable and then the second variable. The alternative is to give the summaries together. So the 2 means, then etc. Maybe a checkbox saying Summaries Together, with the default checked.

Now to the factor by factor:

This is fine as long as I just do the default. But if I press for the Margin then it gives an error.

image

This gives:
image

Once I get this error, it also gives the same error when I untick the Margin - so then it won't do the default table it did before.

I also get an error when I start again than then ask for percentages.

@Vitalis95
Copy link
Contributor Author

@rdstern , have a look at it

@rdstern
Copy link
Collaborator

rdstern commented Oct 24, 2023

@Vitalis95 that's great.
On the Factor by Factor the one item I suggest differently is not to display the summary name column:

image

That's the count_variable - repeated each row. I suggest we just have the table of results.

I am relieved we now seem to have something that seems to work with all the options for factor by factor.

@Patowhiz
Copy link
Contributor

@Vitalis95 if you could resolve @rdstern above comments then I can proceed with my initial review. Thanks.

rdstern
rdstern previously approved these changes Oct 25, 2023
Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@Vitalis95 I am now approving this in its current form.
There is a new pull request needed when @lilyclements helps, to be able to change the order of the summaries and variables in the dialog below.

When that is done, then also could simplufy the label given below to Summaries in Rows

image

This is a great one and long lasting, so delighted when it is merged. I hope @N-thony can check the code soon.

Copy link
Collaborator

@N-thony N-thony left a comment

Choose a reason for hiding this comment

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

Looks good, just some commented codes I think you need to remove them?

instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
instat/dlgDescribeTwoVariable.vb Outdated Show resolved Hide resolved
@Vitalis95
Copy link
Contributor Author

@N-thony , over to you

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@N-thony this still looks ok to me

@N-thony N-thony merged commit 593dee9 into IDEMSInternational:master Nov 3, 2023
2 checks passed
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.

Further improve the Describe > Two Variable Summarise dialogue
5 participants