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

Separate sex and gender; report percentages for 3 categories each #212

Merged
merged 4 commits into from
Jan 3, 2022

Conversation

drfeinberg
Copy link
Contributor

Description

This PR aims at updating the Sex category that previously reported only the number of female participants to deal with both Sex and Gender separately, and provide 3 options each:
Sex: Female/Male/Other;
Gender: Woman/Man/Non-Binary

Proposed Changes

I added new functions for gender, changed the examples to have a gender category, and a 3rd option in each example.

@IndrajeetPatil
Copy link
Member

Thanks a lot!

Can you please also adjust and update the tests here accordingly?
https://github.com/easystats/report/blob/master/tests/testthat/test-report_participants.R

And please also create an entry in NEWS.md to log this change.

@drfeinberg
Copy link
Contributor Author

Hi Indrajeet,
I have made those changes.
Here is what I put in the NEWS...

  • Separated sex and gender into different searches/columns
  • Sex is reported % female, % male, % other, % missing if any cases are missing
  • Gender is reported % Women, % Men, % Non-Binary, % missing if any cases are missing
  • Age reports % missing if any cases are missing
  • Tests have been updated to reflect these changes
  • Tests for missing cases have also been updated to test numbers for NA, and strings for "" as "" is not NA.

It looks like there was to be a discussion about how to approach missing values in the test file. I hope I didn't step on any toes by making some changes. They are well commented so you can see what I did and why.
Best,
David

Copy link
Member

@IndrajeetPatil IndrajeetPatil 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 the thorough treatment.
Expect tests to fail because we still need to make some changes in master branch.

Once @bwiernik also approves, this can be merged.

@DominiqueMakowski
Copy link
Member

Thanks a lot @drfeinberg !

@IndrajeetPatil
Copy link
Member

@bwiernik Can you please have a look at this soon? Thanks.

@drfeinberg
Copy link
Contributor Author

Hey. If it turns out my code is useful and there are other small tasks like this that need getting done, let me know, I'm happy to help out.

@bwiernik
Copy link
Contributor

bwiernik commented Jan 2, 2022

@IndrajeetPatil these changes look good to me. Could you update the tests?

At some point, we should generalize this by taking a named list of matching values. That would let people do their own systems and also, eg, use terms from other languages. Would also work for race/ethnicity, occupation, major, etc.

@IndrajeetPatil
Copy link
Member

The tests to be updated are not relevant for this feature (those have already been updated in the PR itself), so I am merging this.

@IndrajeetPatil IndrajeetPatil merged commit f6949f3 into easystats:master Jan 3, 2022
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.

4 participants