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.

Sign up for GitHub

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

Jump to bottom

gender → sex #212

Merged
merged 3 commits into from
Jul 13, 2023
Merged

gender → sex #212

merged 3 commits into from
Jul 13, 2023

Conversation

DimitriPapadopoulos
Copy link
Contributor

Fixes #207 .

skjerns
skjerns reviewed Jun 3, 2023
View reviewed changes
pyedflib/edfwriter.py Outdated Show resolved Hide resolved
skjerns
skjerns reviewed Jun 3, 2023
View reviewed changes
pyedflib/edfwriter.py Outdated Show resolved Hide resolved
skjerns
skjerns reviewed Jun 3, 2023
View reviewed changes
pyedflib/edfwriter.py Outdated Show resolved Hide resolved
skjerns
skjerns reviewed Jun 3, 2023
View reviewed changes
pyedflib/highlevel.py Outdated Show resolved Hide resolved
@DimitriPapadopoulos
Copy link
Contributor Author

Ouch. Somehow I lost the gender branch. Will retry.

@skjerns
Copy link
Collaborator

skjerns commented Jun 6, 2023

Let me know when you are done then I'll review.

Also: Could you sqash the commits after finishing?

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jun 6, 2023 •
edited
Loading

The new code should be backwards compatible enough. Not sure if I test the backwards compatibility stuff enough.

Can you have a look?

@DimitriPapadopoulos DimitriPapadopoulos marked this pull request as ready for review June 6, 2023 20:49
@skjerns
Copy link
Collaborator

skjerns commented Jun 27, 2023

lgtm

for testing it would be good to also check that the old functions still return the same values.
Easiest would be to assert an assertion with getSex()==getGender() for all the replaced instances in the test suite.

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jul 2, 2023 •
edited
Loading

About "Due to the edf specifications, only binary assignment is possible": I am not a specialist, but I understand most specialists usually describe biological sex as binary, based on what gametes one produces, while fully endorsing gender diversity. See for example https://doi.org/10.1002/bies.202200173. What could be more subjective is the use of biological sex as a variable of interest in situations where it is not relevant.

@DimitriPapadopoulos
Add tests for deprecated setGender()
ace03a7 
Use deprecated getGender() in addition to getSex() in tests.
@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Jul 2, 2023 •
edited
Loading

We do have a similar test in pyedflib/tests/test_edfreader.py:

        np.testing.assert_equal(f.getSex(), 'Male')
        np.testing.assert_equal(f.getGender(), 'Male')  # deprecated

I have just added tests to pyedflib/tests/test_edfwriter.py in test_sex_setting_correctly().

@skjerns
Copy link
Collaborator

skjerns commented Jul 13, 2023

perfect, I think it's ready to merge now :)

@skjerns skjerns merged commit 4084207 into holgern:master Jul 13, 2023
20 checks passed
@DimitriPapadopoulos DimitriPapadopoulos deleted the gender branch July 13, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skjerns skjerns skjerns left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

gender → sex?
2 participants
@DimitriPapadopoulos @skjerns