-
Notifications
You must be signed in to change notification settings - Fork 3
Populating Algorithms: Mogpr from Fusets #80
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
Conversation
@soxofaan @HansVRP
|
algorithm_catalog/fusets_mogpr.json
Outdated
"rel": "openeo-process", | ||
"type": "application/json", | ||
"title": "openEO Process Definition", | ||
"href": "https://raw.githubusercontent.com/ESA-APEx/apex_algorithms/refs/heads/mogpr_v1/openeo_udp/fusets_mogpr/fusets_mogpr.json" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this branch will probably be deleted afterwards, so make sure to update once merged
I'm not sure I understand what your are asking. Is this about raising the issue of lack of documentation? Or just if it is ok to use a single data cube parameter instead of extent parameters? |
in the docs I just see "typically a single node":
so it does not say "it should". At first sight, I think it's ok to have more than one nodes in the benchmark process graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of notes
openeo_udp/fusets_mogpr/set_path.py
Outdated
DEPENDENCIES_DIR2 = 'venv_static' | ||
|
||
DEPENDENCIES_URL1 = "https://artifactory.vgt.vito.be:443/artifactory/auxdata-public/ai4food/fusets_venv.zip" | ||
DEPENDENCIES_URL2 = "https://artifactory.vgt.vito.be:443/artifactory/auxdata-public/ai4food/fusets.zip" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it is very valuable to define these as constants here. Each value is only used once, so you could just use these values directly in the setup_dependencies()
calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the values directly in the setup_dependencies
Could be said such 👀 but No No not exactly. It is a concern on if there is standard way to do as for spatial_extent and temporal extent in terms of naming
Yes, this one esp, because I saw no example of doing such |
as mentioned elsewhere in this issue, I'd recommend in most cases to use
I think it depends basically, It depends on the usage/applicability of the "algorithm": is the algorithm tied closely to a certain data set (e.g. biopar stuff), then it makes sense to include the |
Updated the process similar to that of : https://marketplace-portal.dataspace.copernicus.eu/catalogue/app-details/38 |
openeo_udp/fusets_mogpr/README.md
Outdated
@@ -0,0 +1,39 @@ | |||
# Sentinel-1 and Sentinel-2 data fusion through Multi-output Gaussian process regression (MOGPR) | |||
|
|||
This service is designed to enable multi-output regression analysis using Gaussian Process Regression (GPR) on geospatial data. It provides a powerful tool for understanding and predicting spatiotemporal phenomena by filling gaps based on other correlated indicators. This service focusses on the fusion of Sentinel-1 and Sentinel-2 data, allowing the user to select one of the predefined data sources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
focuses or focusses ?
Finally ready, I guess |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more notes :)
|
||
:param collection: openEO collection parameter | ||
:param label: String representing the text with which the collection should match | ||
:param callable: Function that is executed when the collection matches the label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit confusing: the value you pass to callable
(also called "function" in other places of this script) is not a callable/function but a data cube
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about this one, as the developer seems to mention it as a function everywhere. I will confirm and update it accordingly :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for option in [
{
'label': 'grd',
'function': _load_s1_grd_bands(connection=connection, polygon=polygon, date=date, bands=['VV', 'VH'])
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a matter of syntax, here they pass a callable function which creates a cube.
For me it would be fine to keep things as is since @Pratichhya is not the original developer.
Do you agree @soxofaan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean. _load_s1_grd_bands(...)
is a function call , right, not a callable?
And it's results in a data cube, so you have a dictionary
{
"function": <DataCube Object>
}
which looks weird to me.
But as mentioned elsewhere, don't let all my comments be blockers. I'm fine with accepting things as is, and clean up things later.
@Pratichhya for me it is good to merge, but first rebase to the local main and check that the unit tests are passing |
Rebased and the failure is not with the changes in this branch 😄 |
I'm afraid there is some more work to be done as with #88 the whole folder structure has been changed, so you would have to move some files around in this PR |
indeed, however, it should be simpler now since we have a folder per service! |
This looks like a long chain of changes, thus to address the current change of structure, I migrated the changes to this fresh PR: #95 and closing this PR |
The service is also available at https://marketplace-portal.dataspace.copernicus.eu/catalogue/app-details/17
The modification in the openeo_udp of this repo in comparison to the existing one is: