Populating Algorithms: Mogpr from Fusets

[Pratichhya]

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:

• The dependencies are passed within the files and need not pass separately in the job_options

[Pratichhya]

@soxofaan @HansVRP
Before changing it as Ready from Draft, could you please provide me with your suggestion on:

• Here, the input parameter is only datacube, and there is no definition/example on if there is a restriction to passing this as a parameter instead of spatial and temporal extent as done in other cases.
• In the benchmark scenario, the process graph includes multiple nodes, but according to documentation(correct me if I am wrong) it should only have a single node that points to the namespace.

[HansVRP]

this branch will probably be deleted afterwards, so make sure to update once merged

[soxofaan]

Here, the input parameter is only datacube, and there is no definition/example on if there is a restriction to passing this as a parameter instead of spatial and temporal extent as done in other cases.

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?

[soxofaan]

In the benchmark scenario, the process graph includes multiple nodes, but according to documentation(correct me if I am wrong) it should only have a single node that points to the namespace.

in the docs I just see "typically a single node":

process graph will typically just contain 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

[soxofaan]

a couple of notes

[soxofaan]

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

[Pratichhya]

updated the values directly in the setup_dependencies

[Pratichhya]

Here, the input parameter is only datacube, and there is no definition/example on if there is a restriction to passing this as a parameter instead of spatial and temporal extent as done in other cases.

I'm not sure I understand what your are asking. Is this about raising the issue of lack of documentation?

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

Or just if it is ok to use a single data cube parameter instead of extent parameters?

Yes, this one esp, because I saw no example of doing such

[soxofaan]

It is a concern on if there is standard way to do as for spatial_extent and temporal extent in terms of naming

as mentioned elsewhere in this issue, I'd recommend in most cases to use data for raster cubes, to align with standard openEO process naming. The only standard openEO process that deviates from this, as far as I could think of, is merge_cubes with cube1 and cube2

Or just if it is ok to use a single data cube parameter instead of extent parameters?

Yes, this one esp, because I saw no example of doing such

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 load_collection in the UDP. But if it is a generic algorithm (e.g. producing monthly composites of whatever you feed it), then you can only expect a generic data parameter to get the raster data cube.

[Pratichhya]

#80 (comment)

Updated the process similar to that of : https://marketplace-portal.dataspace.copernicus.eu/catalogue/app-details/38

[SergeCroise]

focuses or focusses ?

[Pratichhya]

Finally ready, I guess

[soxofaan]

some more notes :)

[soxofaan]

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

[Pratichhya]

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 :)

[HansVRP]

    for option in [
        {
            'label': 'grd',
            'function': _load_s1_grd_bands(connection=connection, polygon=polygon, date=date, bands=['VV', 'VH'])
        },

[HansVRP]

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?

[soxofaan]

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.

[HansVRP]

@Pratichhya for me it is good to merge, but first rebase to the local main and check that the unit tests are passing

[Pratichhya]

@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 😄

[Pratichhya]

rebased again and no failure
@soxofaan @HansVRP
Can I go ahead and merge this PR?

[soxofaan]

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

[HansVRP]

indeed, however, it should be simpler now since we have a folder per service!

[Pratichhya]

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