niivue / ipyniivue

A WebGL-powered Jupyter Widget for Niivue based on anywidget
BSD 2-Clause "Simplified" License
25 stars 8 forks source link

Pythonic Way to implement addVolume #20

Closed AnthonyAndroulakis closed 6 months ago

AnthonyAndroulakis commented 1 year ago

Currently, the addVolume function only accepts 1 argument: the path of the volume. However, as can be seen in the niivue docs, addVolume should accept an NVImage object, which includes multiple attributes. The question is how allow for these multiple attributes in a pythonic way.

There are 2 possible methods that immediately come to mind: 1) create an NVImage class in python that can be initialized with all these attributes. This solution might look like:

class NVImage:
    def __init__(
        self,
        data_buffer,
        name,
        color_map,
        opacity,
        paired_img_data,
        cal_min,
        cal_max,
        trust_cal_min_max,
        percentile_frac,
        ignore_zero_voxels,
        visible,
        use_qform_not_sform,
        color_map_negative,
        frame_4D,
        on_color_map_change,
        on_opacity_change
    ):
        self.data_buffer = data_buffer
        self.name = name
        self.color_map = color_map
        self.opacity = opacity
        self.paired_img_data = paired_img_data
        self.cal_min = cal_min
        self.cal_max = cal_max
        self.trust_cal_min_max = trust_cal_min_max
        self.percentile_frac = percentile_frac
        self.ignore_zero_voxels = ignore_zero_voxels
        self.visible = visible
        self.use_qform_not_sform = use_qform_not_sform
        self.color_map_negative = color_map_negative
        self.frame_4D = frame_4D
        self.on_color_map_change = on_color_map_change
        self.on_opacity_change = on_opacity_change

class Niivue:
        ...
        def addVolume(self, image):
               #unpack NVImage object or send the object and somehow deserialize

nv = Niivue()
image = NVImage(name='test.nii.gz', data_buffer='[binary data here]')
nv.addVolume(image)

or 2) spread out the attributes of the NVImage class. This solution might look like:

class Niivue:
        ...
        def addVolume(
            self,
            data_buffer,
            name,
            color_map,
            opacity,
            paired_img_data,
            cal_min,
            cal_max,
            trust_cal_min_max,
            percentile_frac,
            ignore_zero_voxels,
            visible,
            use_qform_not_sform,
            color_map_negative,
            frame_4D,
            on_color_map_change,
            on_opacity_change,
        ):
                #send over arguments to niivue

nv = Niivue()
nv.addVolume('[binary data here]', 'test.nii.gz')

setting default values for each of the attributes might also be good to do

@christian-oreilly

christian-oreilly commented 1 year ago

Sorry for not following up on that earlier! It flew under my radar! So... I like the idea of a class if there are some additional things we want this class to manage (i.e., some methods that operates on these attributes, some argument validation to be coded, e.g., through the @property decorator for setters and getters). I would not do it as a class if it is nothing more than the equivalent of a "C structure" (i.e., just a list of attributes) as shown in example 1. For such a case, the Python dictionary works already quite well, plus they allow the use of the * operator that can be nifty in some case. How familiar are you with the use of the and ** operators to unpack list and dictionaries (when used in the call of a function) or to allow unspecified positional and keyword arguments (when used in the signatures of a function)? Maybe these can be useful? They often are when working with a larger number of arguments.

That is:

args = ['[binary data here]', 'test.nii.gz']
nv.addVolume(*args)

kwargs = {"data_buffer": '[binary data here]', "name": 'test.nii.gz'}
nv.addVolume(**kwargs)

and


        def addVolume(
            self,
            data_buffer,
            name,
            *args, 
            **kwarg
        ):
                # some code

                self.some_other_method_that _requires_the_additional_arguments(*args, **kwargs)

               # some other code

This is quite neat in combination with config files (e.g., json or yaml) which are read as dictionaries that can be passed directly into functions. For a concreate example, you can have a look at these line of codes:

https://github.com/lina-usc/pylossless/blob/14075fc2782d9c5afa67afd897be62f1ccf516fa/pylossless/pipeline.py#L1046-L1060

These lines use the following portion of a YAML config:

https://github.com/lina-usc/pylossless/blob/14075fc2782d9c5afa67afd897be62f1ccf516fa/assets/ll_default_config.yaml#L104-L110

Note that since these arguments are passed to the constructor of the ICA (https://mne.tools/stable/generated/mne.preprocessing.ICA.html), any argument that this constructor accept will work, without me having to define them in my code. Further, the signature of the ICA constructor can change and the users of my code would still be able to use the new arguments available to ICA without me having to change a line of code in my own library.

I am not sure if any of this can be useful for your specific use case; I'll have a better idea when I see what the actual code looks like and how efficient/cumbersome it is.

AnthonyAndroulakis commented 1 year ago

Thank you. I saw your response earlier but I get to implementing these ideas. I'll write a response once I write some implementations.

christian-oreilly commented 1 year ago

Sure thing, @AnthonyAndroulakis