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

Document Forest Change Detection use-case #10

Closed
m-mohr opened this issue Aug 25, 2021 · 21 comments
Closed

Document Forest Change Detection use-case #10

m-mohr opened this issue Aug 25, 2021 · 21 comments
Assignees

Comments

@m-mohr
Copy link
Collaborator

m-mohr commented Aug 25, 2021

No description provided.

@clausmichele clausmichele self-assigned this Aug 26, 2021
@clausmichele
Copy link
Member

Draft of this is available here https://docs.openeo.cloud/usecases/forest-change-detection/
Let me know if you have some feedbacks about it.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Sep 3, 2021

Looks pretty good to me, maybe the code snippets need a bit more contexts. Some variable come out of the blue...

Anyway, I've already added the corresponding JS code in the last days.

I found it interesting that the JS code is so much shorter and was wondering whether this code snippets could be simplified?

args_fit_curve = {
    "parameters": [1,1,1], # Initial guess of the parameters
    "dimension": "t",      # Fit the function along the temporal dimension
    "function": l2a_bands._get_callback(fit_function, parent_parameters=["data","parameters"])
}

curve_fitting = l2a_bands.fit_curve(**args_fit_curve)

@clausmichele
Copy link
Member

clausmichele commented Sep 3, 2021

Thanks for the input. Maybe one of the python client developers can help? @soxofaan @jdries
It would be nice being able to represent the fit function as a string, but I guess it is not possible.

@soxofaan
Copy link
Contributor

soxofaan commented Sep 6, 2021

Indeed, the javascript code is shorter because the fit function there is encoded as a string, which is not supported in Python client. Instead you have to write this python function, which is more verbose.

a small improvement that could be done is skipping the intermediate args_fit_curve variable, like this:

curve_fitting = l2a_bands.fit_curve(
    parameters=[1,1,1], # Initial guess of the parameters
    dimension="t",      # Fit the function along the temporal dimension
    function=l2a_bands._get_callback(fit_function, parent_parameters=["data","parameters"])
)

A couple of things that could be done in the Python client:

  • it's unfortunate that you have to call a private method l2a_bands._get_callback like that, we should make sure that you can just do function=fit_function directly
  • I guess we can also add syntactic sugar so you can write parameters[0] instead of array_element(parameters, 0)

@m-mohr
Copy link
Collaborator Author

m-mohr commented Sep 6, 2021

The use of _get_callback is the most confusing thing here IMHO and should eventually be fixed.
Also, I was wondering about the args_fit_curve object, which could directly be passed as parameters to fit_curve.

@soxofaan
Copy link
Contributor

soxofaan commented Sep 6, 2021

@clausmichele : does that work with

l2a_bands._get_callback(fit_function, parent_parameters=["data","parameters"])

?

I think that should be

l2a_bands._get_callback(fit_function, parent_parameters=["x","parameters"])

because the parameter name "inside" the child function is "x", not "data" (which is the argument name of "fit_curve")

@clausmichele
Copy link
Member

Currently it works, if it should be x instead of data we can correct it after SRR2, Sophie is on holiday right now.

@soxofaan
Copy link
Contributor

soxofaan commented Sep 6, 2021

I already made PR for python client Open-EO/openeo-python-client#230 so that the fit_curve calll can be further simplified to

curve_fitting = l2a_bands.fit_curve(
    parameters=[1,1,1], # Initial guess of the parameters
    dimension="t",      # Fit the function along the temporal dimension
    function=fit_function
)

But I guess we should only proceed with that after SRR2 (because that implementation will use parameter name "x" instead of "data")

@soxofaan
Copy link
Contributor

soxofaan commented Sep 6, 2021

Python client PR https://github.com/Open-EO/openeo-python-client/pull/231will make it possible to write fit_function to something like

def fit_function(x:ProcessBuilder, parameters:ProcessBuilder):
    t = 2 * math.pi / 31557600 * x
    return parameters[0] + parameters[1] * cos(t) + parameters[2] * sin(t)

@soxofaan
Copy link
Contributor

soxofaan commented Sep 6, 2021

side note: I think the documentation should also mention

from openeo.processes import cos, sin, array_element

somewhere, it is otherwise not obvious where these functions come from

@m-mohr
Copy link
Collaborator Author

m-mohr commented Sep 6, 2021

Quick question: In the JS client examples I'm using the openEO process pi() as there it's easier to use - Any reason this is using the Python pi? Thinking about potential advantages or disadvantages...

@clausmichele
Copy link
Member

side note: I think the documentation should also mention

from openeo.processes import cos, sin, array_element

somewhere, it is otherwise not obvious where these functions come from

You're right, I'll fix that

@soxofaan
Copy link
Contributor

soxofaan commented Sep 7, 2021

Any reason this is using the Python pi? Thinking about potential advantages or disadvantages...

using float pi will be marginally more efficient than openeo process pi() call because the backend doesn't need to do an additional process call.
If one is really worried about performance, they can even pre-calculate 2 * math.pi / 31557600, so that it doesn't have to be recalculated with each call of the fit function, e.g.:

_time_scaling = 2 * math.pi / 31557600

def fit_function(x:ProcessBuilder, parameters:ProcessBuilder):
    t = _time_scaling * x
    return parameters[0] + parameters[1] * cos(t) + parameters[2] * sin(t)

That being said, I think the effective performance boost of these kind of optimizations will be negligible in practice.

@jdries
Copy link
Contributor

jdries commented Sep 8, 2021

Also for code like this:

l2a_bands.process("dimension_labels", {
    "data": THIS,
    "dimension": "t"
})

a request should be made to support those dimension_labels and load_result directly in the python client. It's not the intention that users have to work in this ugly generic manner.

@soxofaan
Copy link
Contributor

soxofaan commented Sep 8, 2021

It's not the intention that users have to work in this ugly generic manner.

indeed, and we also have this idea to support processes dynamically in addition to statically defined DataCube methdos, e.g.: Open-EO/openeo-python-client#40 (comment)

@soxofaan
Copy link
Contributor

FYI: Open-EO/openeo-python-client#230 (no need for _get_callback anymore in fit_curve and predict_curve) Open-EO/openeo-python-client#231 (parameter[n] syntax) have been merged

also, DataCube.dimension_labels has been added (Open-EO/openeo-python-client@227ef93)

@clausmichele
Copy link
Member

@soxofaan
Copy link
Contributor

looks a lot better now 👍

minor remark: the fit function is currently called def fitFunction in Python, while it is more common to use snake case def fit_function here

@clausmichele
Copy link
Member

I know, but I just wanted to align between Python and Javascript. It's anyway just a style choice, one may choose to use camelCase for functions and snake_case for variables.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Sep 27, 2021

I think it's fine to use language specifics best practices. Names don't need to 100% fit, so feel free to change to fit_function for Python.

clausmichele added a commit that referenced this issue Jun 30, 2022
fitFunction has been changed to fit_function which is more pythonic.
Closes this #10
@clausmichele
Copy link
Member

I close this, changed from fitFunction to fit_function for Python.
bd07025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants