Draft: Fabio dev

[orianif]

Added functions: eodal.core.band.Band.q_risc() eodal.core.band.Band.im_risc()

example: examples/color_rescaling.py

[lukasValentin]

Hi Fabio,

first, many thanks for your work and for adding this nice feature :+1:

I made some review comments directly in your code but I'd also have some general comments:

1. In the future, please do work on your fork and also create pull request from<your_fork>/dev -> upstream/master. Otherwise the upstream project might get really messy.
2. I think the two functions you created should not be placed in the band.py module. They actually work on RasterCollections (therefore it seems counter-intuitive to place them in the band.py module). As written in one of my e-mails, I'd prefer to have a dedicated module eodal.core.algorithms where functions that take RasterCollection (or band) objects as inputs should go to.
3. Instead of providing a single band_name it would make more sense to me to use a list of band_names so that 1:N bands can be manipulated at once (just add an for-loop to your function). By default, band_names could be None; then all bands are used. By doing so, the second function im_risc becomes obsolete and can be removed.
4. When working on the arrays, make sure to check if the input is a np.ma.MaskedArray or np.ndarray. In the latter case, you're function will fail because np.ndarray has no attribute .data (see also my comments in the code).

In terms of coding style I'd also ask you to improve some things:

• please set types for all function arguments: E.g.,

  def q_risc(rcoll, band_name, qmin=0.01, qmax=0.91, no_data=0):

  should be changed to

  from typing import List, Optional
  
  def q_risc(rcoll: RasterCollection, band_names: Optional[List[str]] = None, qmin: Optional[float] = 0.01, qmax Optional[float] = 0.91, no_data Optional[float] = 0.) -> RasterCollection:

• in the code, please use whitespaces to improve readability and keep the style consice:

  band_out=np.copy(rcoll[band_name].values.data)
  # should be changed to
  band_out = np.copy(rcoll[band_name].values.data)

  Let me know if you have any questions or I can provide any explanations.

[orianif]

Hi Lukas,

Thank you so much for your feedback, I answred below.

------- Original Message ------- On Monday, November 14th, 2022 at 13:15, Lukas Valentin Graf @.***> wrote:

Hi Fabio,

first, many thanks for your work and for adding this nice feature 👍

I made some review comments directly in your code but I'd also have some general comments:

• In the future, please do work on your fork and also create pull request from /dev -> upstream/master. Otherwise the upstream project might get really messy.

Ok, sorry for my git clumsyness XD. Here is what I've done, please tell me what I did wrong:

• I worked on my dev and commited everything to it;
• git pull origin master;
• git merge master (no conflicts)
• git push origin

At this point I expected to see a pull request on github, but I did not see any, but I saw a bloody button proposing to create a pull request for fabio_dev, and I pushed it :D

• I think the two functions you created should not be placed in the band.py module. They actually work on RasterCollections (therefore it seems counter-intuitive to place them in the band.py module). As written in one of my e-mails, I'd prefer to have a dedicated module eodal.core.algorithms where functions that take RasterCollection (or band) objects as inputs should go to.

Ok, I believed in that email you suggested to make q_risc and im_risc methods of the Band and raster classes. Nevermind, I'll make eodal.core.algorithms. It sounds better indeed.

• Instead of providing a single band_name it would make more sense to me to use a list of band_names so that 1:N bands can be manipulated at once (just add an for-loop to your function). By default, band_names could be None; then all bands are used. By doing so, the second function im_risc becomes obsolete and can be removed.

OK I'll merge the two functions, with the option to output a raster cube instead of a rasterCollection. It can be useful to inject them in other packages.

• When working on the arrays, make sure to check if the input is a np.ma.MaskedArray or np.ndarray. In the latter case, you're function will fail because np.ndarray has no attribute .data (see also my comments in the code).

In terms of coding style I'd also ask you to improve some things:

• please set types for all function arguments: E.g.,

def

q_risc

(

rcoll

,

band_name

,

qmin

=

0.01

,

qmax

=

0.91

,

no_data

=

0

):

should be changed to

from

typing

import

List

,

Optional

def

q_risc

(

rcoll

:

RasterCollection

,

band_names

:

Optional

[

List

[

str

]]

=

None

,

qmin

:

Optional

[

float

]

=

0.01

,

qmax

Optional

[

float

]

=

0.91

,

no_data

Optional

[

float

]

=

0.

)

->

RasterCollection

:

• in the code, please use whitespaces to improve readability and keep the style consice:

band_out

=

np

.

copy

(

rcoll

[

band_name

].

values

.

data

)

should be changed to

band_out

=

np

.

copy

(

rcoll

[

band_name

].

values

.

data

)

Ok cool, I'll revise the code accordingly.

Let me know if you have any questions or I can provide any explanations.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[lukasValentin]

Hi Fabio,

I guess the "problem" with your fork was that your origin remote still points upstream. Thus, pushing using origin does not reference your fork.

So the order would be:

1. Check out fork from upstream (has to be done only once)
2. Create two remotes:
   • oirigin points towards your fork (I guess something like https://github.com/orianif/eodal)
   • upstream points towards `https://github.com/EOA-team/eodal
3. push all your developments to origin (git push origin dev)
4. once ready update your local master branch from upstream (git checkout master && git pull upstream master && git fetch upstream --tags)
5. merge master into dev (git checkout dev && git merge master)
6. check your code works and push to origin. Due to the fork relationship the "Create pull request" button will pop-up every time you push to origin/dev

As I said, for the time being it's not a big issue but in the future it will become more and more important :grin:

I'd also have a question about the RasterCube: If that's about developing a new interface than this particular functionality should become a method of the RasterCollection class (RasterCollection.to_rastercube) because it is generic.

Sorry for bothering you with all that stuff :sweat_smile: but I think it's worth it

[orianif]

Hi Lukas,

Ok, I see now what you are proposing. I am writing again the whole process so that it will remain for future.

upstream = official repo branch origin = my github fork dev = my local development branch master = my local master branch

Start by: forking the upstream to your github (via browser) cloning your fork to your computer (git clone ) _ defining two remotes in git: origin and upstream (git remote add )

Then the standard development routine is: --> checkout to dev and work (git checkout dev) --> commit dev with new changes (git commit -am "commit message" ) --> pull upstream to master (git pull upstream master) --> merge master to dev (git merge master) --> if no conflicts, push dev to origin (git push origin dev) a pull request will appear on the official github as well.

Is it correct? :)

Best, Fabio

------- Original Message ------- On Monday, November 14th, 2022 at 14:48, Lukas Valentin Graf @.***> wrote:

Hi Fabio,

I guess the "problem" with your fork was that you're origin remote still points upstream. Thus, pushing using origin does not reference your fork.

So the order would be:

• Check out fork from upstream (has to be done only once)

• Create two remotes:

• oirigin points towards your fork (I guess something like https://github.com/orianif/eodal)

• upstream points towards `https://github.com/EOA-team/eodal

• push all your developments to origin (git push origin dev)

• once ready update your local master branch from upstream (git checkout master && git pull upstream master && git fetch upstream --tags)

• merge master into dev (git checkout dev && git merge master)

• check your code works and push to origin. Due to the fork relationship the "Create pull request" button will pop-up every time you push to origin/dev

As I said, for the time being it's not a big issue but in the future it will become more and more important 😁

I'd also have a question about the RasterCube: If that's about developing a new interface than this particular functionality should become a method of the RasterCollection class (RasterCollection.to_rastercube) because it is generic.

Sorry for bothering you with all that stuff 😅 but I think it's worth it

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

[lukasValentin]

closed in favor of #16