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

[Candidate] Allow null Sex for scanner candidates #7058

Merged

Conversation

CamilleBeau
Copy link
Contributor

@CamilleBeau CamilleBeau commented Sep 23, 2020

Brief summary of changes

Currently, the default value for "Sex" when a scanner is created is NULL. This causes an error when the candidate profile is visited from the family tab (as described in issue #6990). This PR makes it so that a Sex value is not required when selecting a Candidate to accommodate these Scanner candidates.

Testing instructions (if applicable)

  1. Make sure that you can visit the candidate profile of a candidate with entity type scanner
  2. Follow the procedure in the description of [Candidate parameter] Sibling link does not work #6990 and make sure that you do not encounter the error.

Link(s) to related issue(s)

@CamilleBeau CamilleBeau added this to the 24.0.0 milestone Sep 23, 2020
@driusan
Copy link
Collaborator

driusan commented Oct 5, 2020

I don't think this is supposed to be the case. The options for Sex in the candidate table are "enum('Male','Female','Other')". Aren't scanners supposed to be 'Other'? (@cmadjar @nicolasbrossard ?)

@cmadjar
Copy link
Collaborator

cmadjar commented Oct 5, 2020

Right. I believe Sex = Other is (or should be) used for Scanners.

@driusan
Copy link
Collaborator

driusan commented Oct 13, 2020

So can we close this?

@nicolasbrossard
Copy link
Contributor

@driusan @cmadjar I think Other is misleading in the sense that it suggests that the entity is human and has a sex but it's neither Male nor Female. If Other is strictly reserved for scanners, then why not label it something like N/A (scanner)?

@driusan
Copy link
Collaborator

driusan commented Oct 13, 2020

My understanding is that the options map directly from the (required) DICOM PatientSex column where scanners are classified as "O"(ther).

@nicolasbrossard
Copy link
Contributor

nicolasbrossard commented Oct 14, 2020

@driusan Right. Maybe change the label to 'Other (scanner)'? Just to emphasize that when you select 'Other' you are not going to get any humans.

@CamilleBeau
Copy link
Contributor Author

Are we decided on changing the "other" label for sex to "Other (scanner)" for scanners and continuing to not allow null sex?

@driusan
Copy link
Collaborator

driusan commented Oct 30, 2020

No, I don't think "Other (scanner)" makes sense. "Other" does not mean it's a scanner, it means it's not Male or Female. (which happens to include scanners.)

@CamilleBeau
Copy link
Contributor Author

CamilleBeau commented Nov 3, 2020

No, I don't think "Other (scanner)" makes sense. "Other" does not mean it's a scanner, it means it's not Male or Female. (which happens to include scanners.)

@driusan So is the action plan to leave it as other, and change the sex of Scanners to "Other" instead of null so that it can still be mapped from DICOM PatientSex column?

@ridz1208 ridz1208 removed this from the 24.0.0 milestone Nov 27, 2020
@driusan driusan added the Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch label Feb 15, 2021
@driusan
Copy link
Collaborator

driusan commented Apr 22, 2021

@CamilleBeau I don't understand the question. Isn't that already the behaviour that this PR is trying to change it from?

Sorry for the delay in responding.

@CamilleBeau
Copy link
Contributor Author

CamilleBeau commented Apr 23, 2021

@CamilleBeau I don't understand the question. Isn't that already the behaviour that this PR is trying to change it from?

@driusan No this PR is working off of scanners having "null" as sex. I'm just a bit unclear of where we want to go with this based on the conversation in this PR. Do we want scanners to have "null", "other", or a unique scanner sex?

@driusan
Copy link
Collaborator

driusan commented Apr 23, 2021

I think "Other", but maybe it's worth holding off until an imaging meeting.

@CamilleBeau
Copy link
Contributor Author

I think "Other", but maybe it's worth holding off until an imaging meeting.

Ok that sounds good! I'll check in with this soon then.

@CamilleBeau
Copy link
Contributor Author

@driusan Has an imaging meeting happened yet to give more insight on this?

@driusan
Copy link
Collaborator

driusan commented Jun 8, 2021

As far as I know there hasn't been one yet.

@cmadjar cmadjar added the Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties label Jun 8, 2021
@cmadjar
Copy link
Collaborator

cmadjar commented Jun 8, 2021

Will be discussed on Friday. @CamilleBeau let me know if you want to join regarding this bit.

@cmadjar
Copy link
Collaborator

cmadjar commented Jun 11, 2021

Discussed at the imaging meeting of June 11th 2021. Instead of modifying the structure of the table to fix the bug in the candidate parameter module, the candidate parameter module should be updated to allow Other sex.

Note: Scanners have the following sex values which is what is reflected in LORIS to identify the scanner's candidate sex: M (male), F (female), O (other). https://dicom.innolitics.com/ciods/general-ecg/patient/00100040

Closing the PR as discussed during the meeting.

@cmadjar cmadjar closed this Jun 11, 2021
@CamilleBeau CamilleBeau reopened this Jun 16, 2021
@cmadjar cmadjar removed Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Needs Rebase PR contains conflicts with the current target branch or was issued to the wrong branch labels Jul 13, 2021
@driusan driusan merged commit 92e70d0 into aces:main Jul 13, 2021
@ridz1208 ridz1208 added this to the 24.0.0 milestone Aug 24, 2021
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.

[Candidate parameter] Sibling link does not work
6 participants