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

[ANN-760][external] nifti import to multi slot item #549

Merged
merged 31 commits into from
May 10, 2023

Conversation

tomvars
Copy link
Contributor

@tomvars tomvars commented Feb 28, 2023

Currently when a user imports a nifti file using darwin-py it will upload the annotation to the zeroth slot. This PR introduces the ability to specify which slot you are uploading to.

@linear
Copy link

linear bot commented Feb 28, 2023

ANN-760 Nifti import to multi-slot item

Description:

We currently only support nifti imports to MPR view or single volume view. This ticket extends support to multi-slot items, allowing imports to specific slots. It is necessary for Jubilant (50k deal)

Acceptance Criteria:

Copy link
Contributor

@owencjones owencjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I right in saying that broadly the issue here was asynchronicity? Hence the addition of slot names for "reuniting" imported anns afterwards?

If so, nice one, good catch. Might be good to break down the Nifti silo a bit too.

I think mostly comments, but the eval needs careful consideration, as someone putting a wayward value in a JSON might get it to do something unexpected.

darwin/cli.py Outdated Show resolved Hide resolved
darwin/exporter/formats/nifti.py Outdated Show resolved Hide resolved
darwin/exporter/formats/nifti.py Outdated Show resolved Hide resolved
darwin/exporter/formats/nifti.py Outdated Show resolved Hide resolved
darwin/exporter/formats/nifti.py Show resolved Hide resolved
@tomvars tomvars requested a review from owencjones May 4, 2023 15:49
# Sometimes we have a list of lists of AnnotationFile, sometimes we have a list of AnnotationFile
# We flatten the list of lists
if isinstance(parsed_files, list):
if isinstance(parsed_files[0], list):
Copy link
Contributor

@Nathanjp91 Nathanjp91 May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a required change
if not doing anything else with the inner conditional, can flatten this, python supports conditional short circuiting

if isinstance(parsed_files, list) and isinstance(parsed_files[0], list):
    ...

This won't raise if parsed_files isn't indexable because the first condition will short circuit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change actually causes these tests to fail for some reason:

FAILED tests/darwin/importer/importer_mcpu_test.py::Test_find_and_parse::test_runs_single_threaded_if_use_multi_cpu_false - Asse...
FAILED tests/darwin/importer/importer_mcpu_test.py::Test_find_and_parse::test_uses_mpire_if_use_multi_cpu_true - AssertionError:...

Going to merge without it, can't spend any longer on this ticket.

@tomvars tomvars merged commit 6cdf6ed into master May 10, 2023
11 checks passed
@Nathanjp91 Nathanjp91 deleted the ann-760-nifti-import-to-multi-slot-item branch November 8, 2023 10:42
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.

None yet

3 participants