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 random sampling to Probability keyboard in Calculator dialog #7991

Conversation

derekagorhom
Copy link
Contributor

fixes #7953

@rdstern , @N-thony this PR is ready for review

@N-thony
Copy link
Collaborator

N-thony commented Nov 28, 2022

@derekagorhom good, @anastasia-mbithe can you peer review this?

@rdstern
Copy link
Collaborator

rdstern commented Nov 29, 2022

@derekagorhom that's a very good start indeed. Well done. I did say you could use this example to also learn more that would help you generally. When implementing I need you to reflect that your job includes thinking hard on how it will be used - so sometimes improving on what I have written as the issue. Also, making sure you understand the issue well.
a) Keys well laid out with only one spelling I found, namely bernoulli has double l.
b) You have misunderstood the n in (possibly all) the keys I outlined in the issue. They each need a sample of the same size as the runif key - which you coded fine. So the code in runif should be used in the others.
c) You should notice there is a Show Parameters checkbox in the calculator. With most keys we only show the minimum parameters for it to work when that is not checked. Then we show all the parameters, with their default settings, when it is chec ked. That may be relevant for some keys.
d) But for others these keys as examples where it is useful to have the parameters anyway. Then it helps the user to know what to change if they want another instance of that key. So the runif, please add the min = 0 and max = 1 into the ordinary use of the function. So it won't change when the show parameters checkbox is checked, because there is nothing more to add.
e) Similarly for rnorm(n). I now suggest that you add the full set of arguments anyway, i.e. rnorm(n, mean = 0, sd = 1) Of course with your special adjustment for n.
f) Probably something similar for most of the other keys - some you have done already.
g) Then, your own testing should mean you produce or use a data frame and try each key yourself.

@derekagorhom
Copy link
Contributor Author

@rdstern i made the changes, can you please review
thanks

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.

@derekagorhom that is better - and quick too. But I have deduced that you can't have tried your code. You should not expect your tester to be the first person to try the different bits of code you write. I would prefer you to take a bit longer, but be confident that you tested yourself, and critically, before expecting others. Perhaps you were still going to do this and I am testing too soon?
a) My evidence is particularly the poisson key, which doesn't work. I am sure you can discover and correct the error.
b) The uni_integer is a bit more complicated, but try with your default settings and (for example) the survey data, which has 36 observations. That will give an error, because 5 does not go into 36. Then you will discover that you need to swap n and size round.
c) And the sample also doesn't work. There you need to use size, where you have n, and you will have x = , ready for the user, usually to include a variable. (You could alternatively have a number there, and then it is effectively the same as uni_integer.)

@rdstern
Copy link
Collaborator

rdstern commented Nov 30, 2022

Progress - I knew you would be able to combine this task with learning a lot - both about what to do, and also how to do it.
a) Poisson - that was the simple one and you found it! Good.
b) int_uniform. Your version was as follows:
sample.int(size=5, n=nrow(x=survey), replace=TRUE)

I assume you tried it and found it didn't work. Then look at the reference documentation on that command to make the correction - which I thought I had mentioned above. If it did work for you, it is because your data frame was of length a multiple of 5 and you should have noticed that the values keep repeating. It gave an error for me, because the worksheet was of length 36. Changing to
sample.int(n=5, size=nrow(x=survey), replace=TRUE)
c) Now can you change the sample key?

@derekagorhom
Copy link
Contributor Author

Progress - I knew you would be able to combine this task with learning a lot - both about what to do, and also how to do it. a) Poisson - that was the simple one and you found it! Good. b) int_uniform. Your version was as follows: sample.int(size=5, n=nrow(x=survey), replace=TRUE)

I assume you tried it and found it didn't work. Then look at the reference documentation on that command to make the correction - which I thought I had mentioned above. If it did work for you, it is because your data frame was of length a multiple of 5 and you should have noticed that the values keep repeating. It gave an error for me, because the worksheet was of length 36. Changing to sample.int(n=5, size=nrow(x=survey), replace=TRUE) c) Now can you change the sample key?

Thank you @rdstern, i have made the changes to uni_integer and sample

@rdstern
Copy link
Collaborator

rdstern commented Nov 30, 2022

@derekagorhom you are not learning to work as carefully as I hoped. Did you really try the sample key?
a) Here is the documentation on the sample function. I had hoped you would have looked for this?

image

Can you see what is wrong in your code now?
b) And I would then like to simply insert the column to sample from. But the cursor is at the end. It should be after the x = (so not the n = ! And before the comma.

@derekagorhom
Copy link
Contributor Author

hello @rdstern, i have made the changes, can you please check

rdstern
rdstern previously approved these changes Dec 1, 2022
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.

@lloyddewit I am at last ok with this. So over to you.

instat/ucrCalculator.vb Outdated Show resolved Hide resolved
instat/ucrCalculator.vb Outdated Show resolved Hide resolved
@lloyddewit lloyddewit changed the title Added random Sampling to probability keyboard in the calculator Added random sampling to Probability keyboard in Calculator dialog Dec 1, 2022
@lloyddewit lloyddewit merged commit 31638bb into IDEMSInternational:master Dec 1, 2022
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.

Adding random sampling to the probability keyboard in the calculator
4 participants