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

[MED-13][external] nifty export from multi slot item #614

Merged
merged 16 commits into from
Jun 30, 2023

Conversation

NooshinGhavami
Copy link
Contributor

@NooshinGhavami NooshinGhavami commented Jun 12, 2023

Problem

At the moment nifti export only outputs single slot even for multi-slot items in the workview. We want the output to be modified so that annotations are exported per slot

Solution

Modifications were made to the darwin/exporter/nifti.py to handle these modifications and tests/darwin/exporter/export_nifti_test.py new unit tests were added.

Changelog

Modifying code and adding new functionality to be able to handle multi-slot exports for nifti images.

@linear
Copy link

linear bot commented Jun 12, 2023

MED-13 Nifti export from multi-slot item

Description:

We currently support nifti exports from our MPR view as well as single volumes. However we don't support export to nifti for complex multi-slot items which might have multiple volumes.

This is needed for Jubilant a 50k prospect.

Acceptance Criteria:

@owencjones owencjones marked this pull request as draft June 12, 2023 11:13
@owencjones
Copy link
Contributor

@NooshinGhavami have converted to draft whilst these tests are failing. If you need anything reach out on slack.

@tomvars tomvars changed the title Med-13 nifty export from multi slot item [MED-13][external] nifty export from multi slot item Jun 13, 2023
@tomvars tomvars marked this pull request as ready for review June 16, 2023 10:28
@NooshinGhavami
Copy link
Contributor Author

@owencjones tests should be fixed now! Thank you.

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.

I like it, it's really made the flow clearer and more readable, although anything with complex affine geometry is never going to be easy to process mentally at first!

I've made loads of comments, but mostly down to your judgement, the only ones I'd like to require are the type changes.

darwin/exporter/formats/nifti.py Show resolved Hide resolved
darwin/exporter/formats/nifti.py Show resolved Hide resolved
darwin/exporter/formats/nifti.py Outdated Show resolved Hide resolved
darwin/exporter/formats/nifti.py Show resolved Hide resolved
darwin/exporter/formats/nifti.py 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
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.

A few of your functions are still missing return types, but we can ticket that for further improvement later, it's already a nice step forward. Good stuff.

Copy link
Contributor

@tomvars tomvars left a comment

Choose a reason for hiding this comment

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

Looks good to me! We've tested this for a variety of output configurations. It should make its way into our platform for exports next week.

@NooshinGhavami NooshinGhavami merged commit 4460f82 into master Jun 30, 2023
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.

4 participants