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

I/O package use of set_up #871

Open
paskino opened this issue Jun 1, 2021 · 2 comments
Open

I/O package use of set_up #871

paskino opened this issue Jun 1, 2021 · 2 comments
Assignees
Labels
discussion This is a discussion thread for enhancements

Comments

@paskino
Copy link
Contributor

paskino commented Jun 1, 2021

Similarly to what happens with Algorithms here we have a method set_up that is used to set-up the reader/writer class.

While it can be argued that with a simple class that requires just a file name to be passed, all the rest might be overkill, I see that having a single function to configure the class is not nice.

For example the TIFFStackReader may accept just a file name, but it allows the user to do more advanced stuff as defining a roi (with roi and mode parameters) and also transpose the data.
Currently one needs to either pass everything in the initialisation or with set_up:

reader = TIFFStackReader()
reader.set_up(file_name=fname, roi ={'angle': (0, 100, 10)}, method='bin', transpose=False)
#or 
reader = TIFFStackReader(file_name=fname, roi ={'angle': (0, 100, 10)}, method='bin', transpose=False)

data = reader.read()

In fact, I find both not good enough. One should use a more Object oriented (though more verbose) form:

reader = TIFFStackReader()
reader.file_name = fname
reader.roi = {'angle': (0, 100, 10)}
reader.method = 'bin'
reader.transpose = False

The obvious advantage being that the user does not need to reinstantiate a class when just the roi has changed, for instance.

Additionally or better primarily, it is much more maintainable if set_up refers to smaller methods that do the small bits. The docstrings will also be smaller and clearer:

def set_up( self, file_name = None,
               roi = {'axis_0': -1, 'axis_1': -1, 'axis_2': -1},
               transpose = False,
               mode = 'bin'):
    self.file_name = fname
    self.roi = roi
    self.method = method
    self.transpose = transpose
@paskino paskino added the discussion This is a discussion thread for enhancements label Jun 1, 2021
@jakobsj
Copy link
Contributor

jakobsj commented Jun 1, 2021 via email

@paskino
Copy link
Contributor Author

paskino commented Jun 30, 2021

Good points. A few quick questions: 1. Is there a special reason to have a "setup" function instead of "init"? It seems simpler to me that we specify the parameters we want directly when we initialise the object, instead of in multiple lines after that.

I believe the choice here was to try to simplify the __init__ so that it would refer to other functions which do the job

  1. Is it not possible to combine the two approaches of passing as keyword arguments and using the "self.roi = roi" style? For example you can initialise your object with on choice of ROI and then update it later, without calling any setup or init function but just do obj.roi = roi, for an object called obj.

It is possible when self.roi is not a member but a property of the class:

@property
def roi(self):
    return self._roi
@roi.setter
def roi(self, roi):
    # do some checks then save the reference in the class
    self._roi = roi

Of course we have our usual discussion where you like to specify everything in the initialisation and I prefer more verbose, yet clearer IMHO, and more object oriented approaches.

Later we could deprecate set_up really.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This is a discussion thread for enhancements
Projects
None yet
Development

No branches or pull requests

6 participants