Module managment simplify #32
Conversation
src/controller/__init__.py
Outdated
| @@ -0,0 +1,60 @@ | |||
| MODULES = { | |||
There was a problem hiding this comment.
What are the reasons that lead you to change modules configuration save from json to python dict?
I would prefer the use of a json file. You can then load this json inside this init into your MODULES variable.
What do you think?
There was a problem hiding this comment.
Yes we can do that, I removed it because I didnt see the purpose of a json file, since we need python scripting in order to create a new module in any case..
But in fact its useful if we want to personalize the menu.
src/controller/connection.py
Outdated
| if t == "load image": | ||
| widget.browse.clicked.connect(lambda: modules_fn.browse_image(widget)) | ||
|
|
||
| if t == "save image": |
src/controller/modules_fn.py
Outdated
| for need_ref, cbox, function in zip([False, False, True, True], | ||
| [widget.addition, widget.multiplication, | ||
| widget.substraction, widget.division], | ||
| [core.add_images, core.multiply_images, | ||
| core.substract_images, core.divide_images]): |
There was a problem hiding this comment.
It sounds great, but it is a little bit complicated to read, don't you think that using list of need_ref, cbox, function directly could help understand code quickly?.
Or if you prefer to keep this zip to organize it, maybe instantiate something in the file or in the config, to be easier to read?
src/controller/modules_fn.py
Outdated
| for cbox, function in zip([widget.addition, widget.multiplication, | ||
| widget.substraction, widget.division], | ||
| [core.add_value, core.multiply_value, | ||
| core.substract_value, core.divide_value]): |
src/model/core.py
Outdated
| im_sum = im.astype(float) | ||
| im_sum += value |
There was a problem hiding this comment.
Maybe you can factorize it into one line? Like
im_sum = im.astype(float) + value
Or consider the use of numpy.add where you can directly specify a dtype
im_sum = np.add(im.astype(float), value, dtype=...)
There was a problem hiding this comment.
Yep I will use the add function, same for other operations
src/model/core.py
Outdated
| im_sum = im_sum.astype(get_minimum_dtype(im_sum)) | ||
| return im_sum | ||
|
|
||
|
|
||
| def add_images(ims, value=None): |
There was a problem hiding this comment.
If this function is ONLY made for images add, you maybe can remove value from parameters?
src/model/core.py
Outdated
| @@ -95,15 +102,19 @@ def add_images(ims, value=None): | |||
| im_sum: 2d/3d numpy array | |||
| """ | |||
| im_sum = ims[0].astype(float) | |||
There was a problem hiding this comment.
I was wondering reading thos functions, why do you force operations to be made with float array?
There was a problem hiding this comment.
if I add a value to an image of dtype int8, the output image will be also of dtype int8. By converting to float64 I ensure the result to be free of dtype.
Ex: with an image of dtype int8, for one pixel if I have 253 + 5 the result will be 3, not 258.
Maybe I can work around this issue with numpy.add, will check that
There was a problem hiding this comment.
[#36] Lets talk about that on this issue s'il vous plait
src/utils.py
Outdated
| def inner(*args, **kwargs): | ||
| try: | ||
| return foo(*args, **kwargs) | ||
| except ValueError as e: |
There was a problem hiding this comment.
Why did you use this decorator only for ValueError? For the divide by 0?
There was a problem hiding this comment.
No only for empty input, but I anticipate other unknown issues, we will add error we encounter to this except
There was a problem hiding this comment.
0 division is handled with inf/nan values
There was a problem hiding this comment.
In fact it should be a KeyError, I change that
This branch restructure the module management to achieve greater freedom with widget connections.
And add 'rename' in menu to rename nodes (thanks to restructuring).