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

Extended the Calculator Integer keyboard #7826

Merged

Conversation

anastasia-mbithe
Copy link
Contributor

Fixes #7818
This is still in progress. The last part of the issue is yet to be addressed (adding length to the position control).

@rdstern
Copy link
Collaborator

rdstern commented Sep 5, 2022

@anastasia-mbithe this all looks great.
a) Could you add the rep addition to the new prime, e.g. twin keys where needed - as explained in the issue. You already added a similar feature in the prime key, to include the length of the data frame.
b) Add an s to the prime key, so it becomes primes.
c) Move the 5 prime keys from the main set to the top of the new set. Have primes first, then n-th prime and next prime. Then also is prime and coprime.

Many thanks

@anastasia-mbithe
Copy link
Contributor Author

@rdstern , having the rep function for the primes only works for twin and triplets functions though on very limited occasions; when the length of the output is 1.
Its functionality depends on the length of the dataset and the length of the resulting output, so it mostly bugs because the length of the dataset is not divisible by the length of the resulting output from the function.

Even having: rep(primes::third_cousin_primes(0,10000),len=1000) in a dataset of 1000 rows still bugs because the length of the result is 101 which does not divide 1000.

@rdstern
Copy link
Collaborator

rdstern commented Sep 6, 2022

@anastasia-mbithe that's very odd, because it is just what the rep is supposed to do.

I tried with the rep in front of your functions. I used the usual survey, so had 36 and it works fine for all your keys. So, I am not sure what is going on?

For example - from my log file:

calc6 <- rep(primes::sexy_prime_triplets(2 ,10000 ),len=36)
and:
calc7 <- rep(primes::k_tuple( 2,10000 ,c(0,8)),len=36)

image

@anastasia-mbithe
Copy link
Contributor Author

@rdstern , I just added the rep function (am not sure why it was bugging yesterday, today the function works smoothly). I have also set the default to (0,100) for min and max since there has to be a default while implementing the function in this way. Also, am not sure if you want the parameters shown as well; for min, max or tuple.

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.

@anastasia-mbithe the changes are very interesting. Can you show them to @lilyclements . You have included the specific values 0 and 100 for each of these special prime keys. That's different to the other keys. On the other hand it makes it very easy to adapt those values - so I rather like it. It makes those keys easier to use, and I can't see it is doing any harm. So I am inclined to suggest I should approve as it stands. I'd just like Lily to give a second opinion.

lilyclements
lilyclements previously approved these changes Sep 6, 2022
Copy link
Contributor

@lilyclements lilyclements 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 to me!

rdstern
rdstern previously approved these changes Sep 6, 2022
instat/ucrCalculator.vb Outdated Show resolved Hide resolved
instat/ucrCalculator.vb Outdated Show resolved Hide resolved
@rdstern
Copy link
Collaborator

rdstern commented Sep 12, 2022

@lloyddewit and @anastasia-mbithe I do have some further improvements for this keyboard, but will be putting them into a new issue. So it will be good to resolve anything from above and merge first.

@anastasia-mbithe
Copy link
Contributor Author

@lloyddewit, Kindly have a look at the changes made.

@lloyddewit
Copy link
Contributor

@anastasia-mbithe Thank you for the changes, if you can resolve the final open comment above, then I'm happy to approve

@anastasia-mbithe
Copy link
Contributor Author

@lloyddewit, I resolved the pending issue. Kindly have a look at it.

@N-thony
Copy link
Collaborator

N-thony commented Sep 28, 2022

@lloyddewit, I resolved the pending issue. Kindly have a look at it.

@lloyddewit please have look

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.

Great, just some minor comments

instat/ucrCalculator.vb Outdated Show resolved Hide resolved
instat/ucrCalculator.vb Show resolved Hide resolved
instat/ucrCalculator.vb Outdated Show resolved Hide resolved
@anastasia-mbithe
Copy link
Contributor Author

This is also complete as well. The changes requested are already resolved.

@lloyddewit
Copy link
Contributor

@anastasia-mbithe thank you for the changes. I'm happy to approve.

@N-thony thank you for the peer review comments. I checked and they all look resolved. If not, then please let us know.

@rdstern there have been non-functional changes since your last review. Please could you restest/approve? thanks

@N-thony
Copy link
Collaborator

N-thony commented Oct 3, 2022

@anastasia-mbithe thank you for the changes. I'm happy to approve.

@N-thony thank you for the peer review comments. I checked and they all look resolved. If not, then please let us know.

@rdstern there have been non-functional changes since your last review. Please could you restest/approve? thanks

@lloyddewit looks good to me.

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.

Looks good.

@lloyddewit lloyddewit dismissed N-thony’s stale review October 4, 2022 08:10

@N-thony confirmed all comments resolved

@lloyddewit lloyddewit changed the title Extending the Integer keyboard Extended the Calculator Integer keyboard Oct 4, 2022
@lloyddewit lloyddewit changed the title Extended the Calculator Integer keyboard Extendied the Calculator Integer keyboard Oct 4, 2022
@lloyddewit lloyddewit changed the title Extendied the Calculator Integer keyboard Extended the Calculator Integer keyboard Oct 4, 2022
@lloyddewit lloyddewit merged commit 1fcc483 into IDEMSInternational:master Oct 4, 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.

Extend the integer keyboard still further
5 participants