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

WIP: feat 194 class based color settings - bulk annotations #213

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

pedrokohler
Copy link
Collaborator

@pedrokohler pedrokohler commented Jun 10, 2024

Adding the ability to edit annotation groups colors based on their categories and types. Solves #194.

Screenshare.-.2024-06-10.12_43_53.PM.mp4

@pedrokohler pedrokohler changed the title Feat 194 class based color settings WIP: feat 194 class based color settings Jun 10, 2024
Copy link

github-actions bot commented Jun 10, 2024

Visit the preview URL for this PR (updated for commit 559ebce):

https://idc-external-006--idc-slim-preview-kk0jkjse.web.app

(expires Fri, 02 Aug 2024 13:27:32 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 88aacecd98ba54d2f9c8d201a9444e43d1ad8307

@pedrokohler pedrokohler force-pushed the feat-194-class-based-color-settings branch from 4a1ce3a to a3a3f07 Compare June 10, 2024 15:45
@fedorov
Copy link
Member

fedorov commented Jun 10, 2024

I am unable to point the preview instance to a GCP DICOM store - I get this error:

image

@pedrokohler
Copy link
Collaborator Author

pedrokohler commented Jun 14, 2024

I am unable to point the preview instance to a GCP DICOM store - I get this error:

image

@fedorov can you give me an example of a server url you're trying to use?

@fedorov
Copy link
Member

fedorov commented Jun 14, 2024

projects/idc-dev-etl/locations/us-central1/datasets/idc/dicomStores/annotated_rms-original-and-annotations-wc-temp-20240122

@pedrokohler
Copy link
Collaborator Author

projects/idc-dev-etl/locations/us-central1/datasets/idc/dicomStores/annotated_rms-original-and-annotations-wc-temp-20240122

You have to add a /dicomWeb at the end (if you haven't)

When you do that we're getting a 401 error, probably because this dataset requires google authentication but the config file the environment is using does not ask you to login with google.

We're seeing how to change this right now... I'll let you know.

@fedorov
Copy link
Member

fedorov commented Jun 14, 2024

Yes, you also should add the prefix. The full url is this: https://healthcare.googleapis.com/v1/projects/idc-dev-etl/locations/us-central1/datasets/idc/dicomStores/annotated_rms-original-and-annotations-wc-temp-20240122/dicomWeb. I just copied from the console. What I tested and what didn't work for me had both the prefix and suffix.

@igoroctaviano
Copy link
Collaborator

@pedrokohler whats the state on this one?

@pedrokohler pedrokohler force-pushed the feat-194-class-based-color-settings branch 2 times, most recently from 7778418 to 2937dde Compare July 25, 2024 14:23
@pedrokohler pedrokohler force-pushed the feat-194-class-based-color-settings branch 2 times, most recently from 1594960 to 493d19c Compare July 25, 2024 15:46
@pedrokohler
Copy link
Collaborator Author

@igoroctaviano as mentioned in slack I was not able to use the "live" channel id... however we can still try some other custom name. I had used idc-slim-preview for the channel id and it seems to generate a fixed URL, even though it has random bunch of characters at the end, I think it does not change. At least it hasn't with my tests.

We can give it a try and add the URL to the GCP permissions so we can try to authenticate.

@igoroctaviano
Copy link
Collaborator

@igoroctaviano as mentioned in slack I was not able to use the "live" channel id... however we can still try some other custom name. I had used idc-slim-preview for the channel id and it seems to generate a fixed URL, even though it has random bunch of characters at the end, I think it does not change. At least it hasn't with my tests.

We can give it a try and add the URL to the GCP permissions so we can try to authenticate.

Sounds good, please address the tests so we can merge.

@pedrokohler pedrokohler force-pushed the feat-194-class-based-color-settings branch from 493d19c to 66c758d Compare July 26, 2024 12:45
@pedrokohler pedrokohler force-pushed the feat-194-class-based-color-settings branch from 66c758d to 170dca8 Compare July 26, 2024 12:59
@pedrokohler
Copy link
Collaborator Author

@fedorov this can now be tested with the dataset you required. Please do and let us know if we can merge.

@fedorov
Copy link
Member

fedorov commented Jul 26, 2024

Per discussion in slack, it appears that currently this feature supports coloring of the annotation groups that come from bulk annotation modality (ANN). It should be able to handle annotations that come from SR, and allow per type/category coloring.

This is a representative sample that can be used to drive the development: https://idc-external-006--idc-slim-preview-kk0jkjse.web.app/studies/2.25.155017484756498730492136597238994838876/series/1.3.6.1.4.1.5962.99.1.3434988325.342625608.1687062201125.4.0.

@fedorov
Copy link
Member

fedorov commented Jul 26, 2024

Per discussion in slack, I suggest we do the following. Let's shelve this current PR for now, and start a new PR that adds these features for SR annotations, into a dedicated new section, reusing as much from the current PR as makes sense. At a later time, when there is a need, we can come back to the ANN PR, and either add its features into a new section, or merge with the SR annotations section.

As @pedrokohler said, it's a good compromise... it won't make us waste what we have and we can make progress without much difficulty.

@fedorov fedorov changed the title WIP: feat 194 class based color settings WIP: feat 194 class based color settings - bulk annotations Aug 29, 2024
Copy link

sonarcloud bot commented Nov 20, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
32.8% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@igoroctaviano
Copy link
Collaborator

@pedrokohler whats the status of this pr? can we close it?

@pedrokohler
Copy link
Collaborator Author

@pedrokohler whats the status of this pr? can we close it?

Same status as the one explained in Andrey's last comment. I don't think it's supposed to be closed.

@igoroctaviano
Copy link
Collaborator

@pedrokohler if this is going to be open/maintained I suggest updating it from time to time and addressing conflicts as soon as they happen.

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.

3 participants