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

Implemented the drop unused levels checkbox in the Column Summaries dialog #8986

Merged
merged 13 commits into from
Sep 12, 2024

Conversation

N-thony
Copy link
Collaborator

@N-thony N-thony commented May 23, 2024

Fixes #8730
@rdstern I made the change as requested for the drop unused levels checkbox. @lilyclements can now make the change in the calculation system.

@rdstern
Copy link
Collaborator

rdstern commented May 23, 2024

@lilyclements I am under the impression you have the change in R, but had "hard-wired it, in your version to test. I hope it could now be included in this pull request, as the condition that the by variables have to be factors is now there. The checkbox should be disabled if they are not.

@rdstern
Copy link
Collaborator

rdstern commented May 29, 2024

@lilyclements can you incorporate your new code in this pull request, so the option can be completed?

@rdstern
Copy link
Collaborator

rdstern commented Jun 14, 2024

@lilyclements are you ok to add your magiv here. You said you had the solution, but "hardwired". Note it will be needed again later in the Tables dialog.

@lilyclements
Copy link
Contributor

@lilyclements are you ok to add your magiv here. You said you had the solution, but "hardwired". Note it will be needed again later in the Tables dialog.

@rdstern as you pointed out the changes are in #8794 - thanks!

@N-thony
Copy link
Collaborator Author

N-thony commented Aug 28, 2024

@rdstern I have added #8794 from @lilyclements into this PR, can you test?

@rdstern
Copy link
Collaborator

rdstern commented Aug 29, 2024

@lilyclements I am unable to test changes just now (misplaced laptop!). It would be great if this now works. Could you test?

@rdstern
Copy link
Collaborator

rdstern commented Sep 4, 2024

@lilyclements has agreed to test this. Then, or in parallel, could @Patowhiz check the code and merge. This is an important new feature in the calculation system. (It is also helping an improvement in the climatic dialogs.)

@rdstern
Copy link
Collaborator

rdstern commented Sep 6, 2024

@N-thony if I understand correctly, then the check unused levels should not be disabled. Let me give an example to check the different options now are possible,
In the survey dataset, if you summarise yield by both the variety (3 levels) and village (4 levels), then you get a summary data frame with either 10 or 12 (3 times 4) rows.

image

In the past there were always 10 rows, because 2 combinations have zero frequencies. (NEW variety is only in 2 of the 4 villages.)

image

Now, with the @lilyclements and your changes, the user can choose - depending on the state of the Drop Unused Levels checkbox.

So if you don't drop the unused levels then I think you should now get the following table.

image

I suggest the 10 rows remain the default, so the Drop Unused Levels checkbox is checked by default.

@N-thony
Copy link
Collaborator Author

N-thony commented Sep 6, 2024

@rdstern @lilyclements Drop Unused Levels checkbox is now enabled

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.

@N-thony looks good! Approving now, @Patowhiz over to you

@rdstern
Copy link
Collaborator

rdstern commented Sep 7, 2024

@Patowhiz this is a small, but important improvement. I'll explain when we meet. It links to "your" tables too! Hope the code is ok, so it can be merged.

@rdstern
Copy link
Collaborator

rdstern commented Sep 8, 2024

@Patowhiz I am back testing again and very happy with this new feature. Over to you to check and merge.

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.

Thanks for reviewing this @lilyclements

@Patowhiz Patowhiz merged commit b1915f8 into IDEMSInternational:master Sep 12, 2024
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.

Implement the Drop Unused Levels checkbox in the Column Summaries dialog
4 participants