refactor: use modern python development pardigm #18

12rambau · 2023-10-31T17:19:03Z

I removed the files that are not making senss (empty)
I recoded some of the methods
I wrote down the documentation for every function

TODO

I already reviewed the helper.py file and I started to look at the layers.py one. There is a lot of duplicated code. As refactoring is a painful process I would like to make sure that this is needed. If not let me know and we'll try to get rid of the one that are done twice.

@rodrigo-ldc could you help me on this?

rodrigo-ldc · 2023-10-31T18:07:56Z

I guess by modern python development paradigm you mean using dataclasses. To be honest, I've never seen it before, looks cool but it's something I need to get used to. Getting rid of __init__ is a big step for me.

rodrigo-ldc · 2023-11-01T11:43:44Z

When I was about to finish reviewing helpers.py I realized that many of the functions there were replaced by methods in layers.VisParams. I'm sorry @12rambau if you find this confusing, this was a WIP and I put it in GitHub as it was 'cos I wanted the rest of the team to benefit from it, and use it in development. Should've clean it up before uploading, what I was in a hurry and decided to do it after

12rambau · 2023-11-01T13:39:45Z

I guess by modern python development paradigm you mean using dataclasses. To be honest, I've never seen it before, looks cool but it's something I need to get used to. Getting rid of __init__ is a big step for me.

If you look at the PR I also changed numerous things that were not available prior to Python 3.7 one-liner extention of dict, no init files, dataclasses so that's a bit more than just replacing files.
Also note that my objective is to change nothing on how it is working

rodrigo-ldc · 2023-11-01T22:43:29Z

Yeah, don't worry, improvements in behavior are also welcome 😃 I even have a todo list, like .getBounds(), .setBounds, centerObject! In fact, I'm thinking in refactor (improve) the messaging behavior, 'cos now it's only working for adding layers to the map, but my general idea is that the message should contain the method that it'll call and the params that will use for that method, what do you think?

12rambau · 2023-11-02T07:59:38Z

Yeah, don't worry, improvements in behavior are also welcome 😃 I even have a todo list, like .getBounds(), .setBounds, centerObject! In fact, I'm thinking in refactor (improve) the messaging behavior, 'cos now it's only working for adding layers to the map, but my general idea is that the message should contain the method that it'll call and the params that will use for that method, what do you think?

I think all these ideas are good ideas but they are out of scope for this PR. Could you open individual issues for each one of them so we can discuss implementation on case by case ?

rodrigo-ldc

hey @12rambau, I reviewed almost all modification. I might need to give a final lookup before approving. Also we can discuss on my comment. Thanks

rodrigo-ldc · 2023-10-31T18:15:53Z

geeservermap/map.py

-    def _addImage(self, image, visParams=None, name=None, shown=True, opacity=1):
-        """Add Image Layer to map."""
-        vis = layers.VisParams.from_image(image, visParams)
-        image = layers.Image(image, vis)
-        layer = image.layer(opacity, shown)
-        data = layer.info()
-        data["name"] = name


sorry @12rambau, but I don't agree much on this, which also relates to changes in lines 25 and 26.. addLayer is a proxy method to add different kind of layers, as in the code editor, the idea is to be able to add other types as FeatureCollection, Geometry, etc.. and I think having all procedures in the same method inside if..else blocks is less convenient than having one hidden method for each case (_addImage, _addFeatureCollection, etc)

rodrigo-ldc · 2023-10-31T18:22:12Z

geeservermap/layers.py

-            print("opacity cannot be less than 0, setting to 0")
-            opacity = 0
-        self.opacity = opacity
+        self.opacity = max(min(opacity, 1), 0)


this way we loose the benefit of telling the user what went wrong (lines 123 & 126)

TBH I think it isn't worth the extra lines, this clamp mechanism is super simple to read and it will reduce the number of if statements to check in the coverage. Is it a problem if when a user sets opacity to 14, we silently agree it's dum and set it to 1?
In your implementation the map is displayed anyway so apart from advanced users nobody will see the actual log.

🤔 I don't think only advanced users will see it, since it's not going to the flask log but to the interpreter stdout (considering users read it, which is not always the case)... maybe we could use the logging module. We could also reduce the number of if statements by checking both greater and lower at the same time, like

if opacity > 1 or opacity < 0: print("opacity cannot be greater than 1 or lower than 0, setting to 1") opacity = 1

I know is an extra statement just to send some message to the user.. I guess it's a matter of deciding where we consider the trade off

rodrigo-ldc · 2023-10-31T18:45:55Z

geeservermap/helpers.py

-    if isinstance(bands, list):
-        bands_list = bands
-        bands_str = visparamsListToStr(bands)
+    return ",".join([f"{b}" for b in viz_params])


this kind of makes sense from a python perspective, but it's not the needed solution. The issue arises when passing a two elements list, then it should only use the first one. When passing more than 3 elements, then I should think more in depth 🤔 the code says use only the first but maybe we could use the first 3 elements. This would require some testing.

for the current approach I'd suggest

if 0 < n < 3 : newbands = f"{params[0]}" else: newbands = f"{params[0]},{params[1]},{params[2]}"

what about something like this ? (I think the numbers re wrong I always need to double-check these things)

slice = 3 if n >= 3 else 1 return ",".join([f"{b}" for b in viz_params[:slice])

the output is the same, and is a bit less readable to me.. but I'm ok

rodrigo-ldc · 2023-10-31T18:46:21Z

geeservermap/helpers.py

    Returns:
        a string formatted as needed by ee.data.getMapId
    """
-    if not params:


this was for handling empty lists

rodrigo-ldc · 2023-11-01T10:59:10Z

geeservermap/helpers.py

-    for band in bands:
-        proxy_bands.append(band.strip())
-    return proxy_bands
+    return [b.strip() for b in viz_params.split(",")]


here, as this is not used by the user, I agree with the list comprehension

rodrigo-ldc · 2023-11-01T11:57:10Z

geeservermap/layers.py

-    @staticmethod
-    def __format_bands(bands):
-        """TODO Missing docstring."""
+    def __format_bands(self, bands: Union[str, list]) -> list:


I think this doesn't need to be attached to self, in fact, as it is here breaks the factory method VisParams.from_image. I could've used the functions in helpers.py but since some funcions there where exclusively to used in viz params, like visparamsListToStr, I decided to put them as static methods of layers.VisParams

rodrigo-ldc · 2023-11-01T12:00:08Z

geeservermap/layers.py

-        else:
-            bands = visParams["bands"]
+        bands = visParams.get("bands", image.bandNames().getInfo())
        bands = cls.__format_bands(bands)


this is failing 'cos __format_bands is no longer an static method

rodrigo-ldc · 2023-11-01T12:06:04Z

geeservermap/layers.py

-        if "bands" not in visParams:
-            bands = image.bandNames().getInfo()
-        else:
-            bands = visParams["bands"]


this is the same issue as in helpres.py. The purpose of this was to avoid fetching the band names if the user provides them. I'd agree on doing

try: bands = visParams["bands"] except KeyError: bands = image.bandNames().getInfo()

but using dict.get will fetch every time

why is it a problem to fetch every time ? you worry about computation cost/time ?

exactly, cost/time

rodrigo-ldc · 2023-11-01T12:19:00Z

geeservermap/layers.py

-            bands = visParams["bands"]
+        bands = visParams.get("bands", image.bandNames().getInfo())
        bands = cls.__format_bands(bands)
-        visParams["bands"] = bands


erasing this line makes previous lines useless

geeservermap/layers.py

12rambau added 5 commits October 31, 2023 12:33

refactor: remove never used file

d6d3c10

refactor: remove redunding file

7ce6557

refactor: set layer as a normal package

228de5a

refactor: remove redundant statement

7dbc9bf

refactor: first batch of refactoring

a6c3525

12rambau marked this pull request as draft October 31, 2023 17:19

12rambau requested a review from rodrigo-ldc October 31, 2023 17:19

rodrigo-ldc reviewed Nov 8, 2023

View reviewed changes

refactor: use modern python development pardigm #18

Are you sure you want to change the base?

refactor: use modern python development pardigm #18

Uh oh!

Conversation

12rambau commented Oct 31, 2023

TODO

Uh oh!

rodrigo-ldc commented Oct 31, 2023

Uh oh!

rodrigo-ldc commented Nov 1, 2023

Uh oh!

12rambau commented Nov 1, 2023

Uh oh!

rodrigo-ldc commented Nov 1, 2023

Uh oh!

12rambau commented Nov 2, 2023

Uh oh!

rodrigo-ldc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!