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

Ensured that Stem and Leaf plots work correctly in Describe > One Variable > Frequencies dialog #8375

Merged
merged 27 commits into from Jun 23, 2023

Conversation

Fidel365
Copy link
Contributor

fixes partially issue #7980
For part two of the issue, an error occurs when we replace the single receiver with a multiple receiver ; 'x must be a numeric value', this occurs when we use 2 or more variables in the multiple receiver.
@rdstern , this PR is ready for review

rdstern
rdstern previously approved these changes Jun 14, 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.

@Fidel365 this now seems to work - at least with a single variable. I am approving and would be happy for this to be merged as it is.

Maybe you can also leave the issue open, in case it is not too difficult to include the multiple receiver. It will need someone to change the R-code. Could you ask @EstherNjeriLiberatta first to see if she can do it easily. Emphasise that she should not spend more than 15 minutes on it - it will depend a) on her R skills and b) If it really is easy. Then I hope @lilyclements can check her suggestions. If it is difficult, then we leave it. But I think it may be easy, and also instructive.

@rdstern rdstern changed the title Describe one variable Describe one variable Frequencies for Stem and Leaf plots Jun 17, 2023
@lilyclements
Copy link
Contributor

@Fidel365 let me know when this is ready for me to check and we can go through together

@Fidel365
Copy link
Contributor Author

@rdstern , this PR is ready for a re-review
But, we have to filter variables for the stem and leaf to be numeric and not for the Table and Graph; Hence when we set a datatype, we do not have a way of removing it; If we filter for numerics in stem and leaf the change applies also to Table and Graph, there should be an issue addressing this; i'll create one

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.

@fidel, this is looking pretty good now, and I really like the way you are explaining some of the limitations you have found.
a) In the old version - when it was a single receiver - it did just what you wanted, namely only the numeric variables are available below for stem and leaf, but not for the other options. Why is that so difficult now?

image

And here is the output in Version 0.7.6 - so from a year ago.

image

Could we return and have that same, nice clean, output now - ideally still for multiple (numeric variables, but without the line numbers and the quotation marks? Maybe check with @Patowhiz.

However, I note that the 0.7.6 output does not say which variable it is summarising. It would be good to add that, particularly now that multiple variables are bing processed.
.

@Fidel365
Copy link
Contributor Author

@rdstern , the issue with the quotes in the stem and leaf has been fixed, but not for the line numbers, @lilyclements says it's not possible

@Fidel365
Copy link
Contributor Author

@rdstern , this PR is ready for a re-review

rdstern
rdstern previously approved these changes Jun 22, 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.

@Fidel365 and @lilyclements I am really sorry I suggested this.

It is important we have a working stem and leaf, but the earleir one variable at a time was ok. I just assumed it would be a minor change to be able to have a multiple receiver there!

How wrong I have been! There are now 2 problems,
a) We can't restrict to just numeric - which we had with one variable
b) The output is acceptable, but messier than with one variable.

image

I am approving, because it is working ok now. But, I am very soory to say that changing back to the one variable is a lesser of evils.

I hope that it doesn't take @lloyddewit too much time, so this can be merged for now. Then my suggestion is to revert to single variable, neat output, and just numeric!

@lloyddewit lloyddewit changed the title Describe one variable Frequencies for Stem and Leaf plots Improved Stem and Leaf plots in Describe > One Variable > Frequencies dialog Jun 23, 2023
@lloyddewit lloyddewit changed the title Improved Stem and Leaf plots in Describe > One Variable > Frequencies dialog Ensured that Stem and Leaf plots work correctly in Describe > One Variable > Frequencies dialog Jun 23, 2023
@lloyddewit lloyddewit added the bug label Jun 23, 2023
Copy link
Contributor

@lloyddewit lloyddewit left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. Only 2 small suggestions.

Private clsGraphSjGGFreqPlotRFunction, clsGraphGridRFunction, clsGraphGridAsGGplotRFunction As New RFunction

'stem and leaf functions
Private clsStemLeafNoQuotes, clsStemLeafCaptureOutputFunction, clsStemLeafPurrMapRFunction, clsStemLeafRFunction As New RFunction
Copy link
Contributor

Choose a reason for hiding this comment

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

consistent naming?

Suggested change
Private clsStemLeafNoQuotes, clsStemLeafCaptureOutputFunction, clsStemLeafPurrMapRFunction, clsStemLeafRFunction As New RFunction
Private clsStemLeafNoQuotesRFunction, clsStemLeafCaptureOutputFunction, clsStemLeafPurrMapRFunction, clsStemLeafRFunction As New RFunction

ucrChkGraphFlipCoordinates.SetRDefault("FALSE")

'----------------------------------
'stema and leaf controls
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'stema and leaf controls
'stem and leaf controls

@lilyclements
Copy link
Contributor

@rdstern there's a bit of an issue with this function, and that's that it prints the item rather than stores it. It wasn't picked up on before, but this has always been an issue. @Patowhiz looked into it to find a way to "store" it, and unfortunately that's why the output is not as "clean" now. By storing it, we are enabling it to be viewed later on under bits like "View Objects".
So we have a choice: Would we rather have it as an object, and so viewable later on and stored within R-Instat
Or would we rather just have it print now, and un-saveable?

Maybe to help show, then run this:

# if we save it as an object (here "hello") it still runs the code! This is not like how R usually works!
hello <- stem(x = 1:100)

#  The decimal point is 1 digit(s) to the right of the |
#
#  0 | 123456789
#   1 | 0123456789
#   2 | 0123456789
#   3 | 0123456789
#   4 | 0123456789
#   5 | 0123456789
#   6 | 0123456789
#   7 | 0123456789
#   8 | 0123456789
#   9 | 0123456789
#  10 | 0
#

If I then call my object, I get no output, just NULL:

hello
NULL

@rdstern
Copy link
Collaborator

rdstern commented Jun 23, 2023

@lilyclements and @Fidel365 my main interest now is to finish this task using the minimum of your time. So we can exist with the current printout - so it is stored.

Should we return to the single receiver so we can restrict the variables to numeric?

@lilyclements
Copy link
Contributor

lilyclements commented Jun 23, 2023

@rdstern great - @Fidel365 is working on it now. Thanks

@Fidel365
Copy link
Contributor Author

@rdstern , this is ready for review
The multi receiver for stem and leaf now only picks numerics

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.

@Fidel365 well done. @lloyddewit over to you

@lloyddewit lloyddewit merged commit 980819d into IDEMSInternational:master Jun 23, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants