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

Volumes modules #747

Merged
merged 9 commits into from
Oct 16, 2023
Merged

Volumes modules #747

merged 9 commits into from
Oct 16, 2023

Conversation

EmmaRenauld
Copy link
Contributor

Preparing a few other refactored modules.

Goal: To merge this before the scil retreat so that people can do the unit tests.

Summary:

  • image.utils --> Stayed the same for now. They are quite 'utils' as we discussed.
  • utils.image --> now image.volume_operations (as discussed). Ex: flip volume, crop volume, apply a transform, etc. However that name already existed:
  • image.volume_operations --> now image.volume_math. They are all the math operations from our math script.
  • image.datasets --> now image.volume_space_management. To be cleaned another day but at least name is more explicit.

Associated scripts:
The following scripts are now clean (i.e. the main script is as empty as possible).

  • scil_apply_transform_to_image
  • scil_count_non_zero_voxels
  • scil_resample_volume
  • scil_reshape_to_reference
  • scil_snr_in_roi
  • scil_flip_volume
  • scil_image_math
  • scil_crop_volume

@arnaudbore arnaudbore self-requested a review September 21, 2023 17:53
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

Copy link
Collaborator

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

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

Small discussion to have on WorldBoundingBox and its usage in scil_crop_volume.py

from scilpy.utils.util import voxel_to_world


class WorldBoundingBox(object):
Copy link
Collaborator

@AlexVCaron AlexVCaron Sep 21, 2023

Choose a reason for hiding this comment

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

We need to discuss this more. This class is used as a pickle in the scil_crop_volume.py script. Moving it here won't work, it must be a top of module to be picklable (https://docs.python.org/3/library/pickle.html).

And even, it will bug all pickled WorldBoundingBox kept by scilpy user (a pickle is bound to the module in which it was created, which is main up till now). As such, the script parameters --input_bbox won't work with old boxes.

If we intend to deprecate those boxes, we should deprecate this pickle usage instead and rely on another saving method for the bounding box. It is legacy and really the worst way of doing this job.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@arnaudbore @frheault @EmmaRenauld I would move this code back to the script, if we all agree. With a todo to change the way it works, remove pickling from the solution.

Copy link
Contributor Author

@EmmaRenauld EmmaRenauld Sep 21, 2023

Choose a reason for hiding this comment

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

Ok.

@frheault , I will remove scil_crop_volume from files to give to people at the retreat. Seems more complicated than expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexVCaron Tests were passing, though. Do you have an example of pickle file that we could add to the tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do not test the input and output pickle, so we don't check this behavior. I'll prepare a little package for here and the retreat.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is some data. You have the pickled bbox, created from the current master.

Loading the box does not work. However, contrary to what I thought, it is still possible to create and load boxes with the bbox class in the module. I still think we should change to another way than pickle, but we'll talk about it at the retreat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I will try to play with this eventually.

scilpy/tracking/propagator.py Outdated Show resolved Hide resolved
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@EmmaRenauld
Copy link
Contributor Author

(Rebased on master. Locally tests are passing).

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@EmmaRenauld
Copy link
Contributor Author

@frheault @AlexVCaron @AntoineTheb This is ready!

Copy link
Collaborator

@AlexVCaron AlexVCaron left a comment

Choose a reason for hiding this comment

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

I have not tested, but everything looks good to me ! If you want to, LGTM.

@arnaudbore arnaudbore merged commit bd62f0c into scilus:master Oct 16, 2023
@EmmaRenauld EmmaRenauld deleted the volume_module branch November 2, 2023 17:29
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