Skip to content

gender → sex#212

Merged
skjerns merged 3 commits intoholgern:masterfrom
DimitriPapadopoulos:gender
Jul 13, 2023
Merged

gender → sex#212
skjerns merged 3 commits intoholgern:masterfrom
DimitriPapadopoulos:gender

Conversation

@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor

Fixes #207 .

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the gender branch 8 times, most recently from aabef26 to 5477e90 Compare June 2, 2023 21:56
@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor Author

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

@DimitriPapadopoulos DimitriPapadopoulos force-pushed the gender branch 5 times, most recently from d506586 to f349cd9 Compare June 5, 2023 18:20
@skjerns
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

DimitriPapadopoulos commented Jun 6, 2023

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
Copy Markdown
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
Copy Markdown
Contributor Author

DimitriPapadopoulos commented Jul 2, 2023

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.

Use deprecated getGender() in addition to getSex() in tests.
@DimitriPapadopoulos
Copy link
Copy Markdown
Contributor Author

DimitriPapadopoulos commented Jul 2, 2023

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
Copy Markdown
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
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

gender → sex?

2 participants