-
Notifications
You must be signed in to change notification settings - Fork 41
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
Fix gee example #128
Fix gee example #128
Conversation
@@ -52,7 +52,14 @@ def bands(self) -> List[Band]: | |||
|
|||
def _get_bands(self) -> List[Band]: | |||
# TODO refactor this specification specific processing away in a subclass? | |||
# First try `properties/eo:bands` | |||
# First try `summaries/eo:bands` | |||
eo_bands = self.get('summaries', 'eo:bands') |
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.
ah ok, I didn't know about this summaries
field as replacement for the properties
field.
Maybe we can drop support for properties
in favor of summaries
. I have no idea if it is necessary to keep supporting that
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.
Yes, also "cube:dimensions" is now one layer up in the dictionary, I updated that in my last commit.
if self._api_version.at_least("1.0.0"): | ||
request = {"process": {"process_graph": graph}} | ||
else: | ||
request = {"process_graph": 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.
reference for self: Open-EO/openeo-python-driver#33
"title": title, | ||
"description": description, | ||
"plan": plan, | ||
"budget": budget | ||
} | ||
|
||
if self._api_version.at_least("1.0.0"): | ||
pg_meta["process"] = {"process_graph": 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.
reference for self: Open-EO/openeo-python-driver#33
openeo/rest/datacube.py
Outdated
if isinstance(process, str): | ||
arguments["process_graph"] = PGNode( | ||
process_id=process, | ||
arguments={data_argument: {"from_parameter": "x"}}) |
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 0.4 it has to be from_argument
I think
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.
Yes true, done in latest commit
openeo/rest/datacube.py
Outdated
)) | ||
|
||
def reduce_temporal_simple(self, process_id="max") -> 'DataCube': | ||
def reduce_temporal_simple(self, process_id="max", dim_abbr="temporal") -> 'DataCube': |
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 would use dimension
instead of dim_abbr
, as used in other places in DataCube methods
also, picking the correct dimension should be done automatically instead of manually by user: see #116. Can you put a comment # TODO #116 determine dimension based on datacube metadata
here as well?
openeo/rest/datacube.py
Outdated
@@ -711,6 +725,10 @@ def merge(self, other: 'DataCube', overlap_resolver: PGNode = None) -> 'DataCube | |||
'cube2': {'from_node': other._pg}, | |||
} | |||
if overlap_resolver: | |||
# TODO: for 1.0.0 support |
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.
Can you add #125
in this TODO line? makes it easier to find 1.0 related TODOs
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.
or wait: as far as I understand the spec, there is no need for additional process
level here. I think this is correct:
...
"overlap_resolver": {
"process_graph": {
"max": { "process_id": "max" ....
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.
Yes you are right, I removed the comments
openeo/rest/datacube.py
Outdated
process_id=func, | ||
arguments={"data": {"from_parameter": "data"}} | ||
)} | ||
# TODO: Might be necessary for 1.0.0rc2 |
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.
same as above: from my understanding of spec this additional "process" level is not necessary
openeo/rest/job.py
Outdated
""" Download job results.""" | ||
def download_results(self, target: Union[str, pathlib.Path] = None) -> list: | ||
""" | ||
Download job results to the target folder path. |
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 feels like a confusing API (especially the interplay between target
and how many files there are in the result)
maybe we should provide instead:
download_result
to download a single result: fails when target is a folder or there are multiple resultsdownload_results
is more generic: target should always be a folder
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.
Now there are these two functions in the job class.
return target | ||
|
||
file_list = [] | ||
|
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.
put a line target = Path(target or Path.cwd())
here, then you don't have to do if target
anymore in the code below
openeo/rest/job.py
Outdated
handle.write(block) | ||
return target | ||
|
||
file_list = [] |
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.
isn't it more logical to use a "to download" dict (mapping hrefs to local paths) instead of a list file_list
?
to address all "process"/"process_graph" changes I created dedicated ticket #129 |
Yes sure that is a good idea. |
- based on Open-EO#128 - more flexible file/folder handling - automatically create parent folder - support multiple result files: `download_results` vs `download_result` - add unit tests
FYI: I just merged a rework of this PR in master |
Hi @bgoesswe I think I merged now all your changes (with some finetuning here and there) into master (including commit Because of the finetuning, this PR #128 is conflicting, and I think it is best to close it. If you have more additions it is probably best to start with a new branch starting from master |
another thing (one of the finetunings I did), is simplification of this pattern:
you can now just do
instead of a hardcoded sleep, it will do a poll loop until job is finished |
Yes works fine, thanks ! |
…pectral_bands", "temporal") - bump version so users can stick to older client version if dimension name checks are too aggressive for their backend - recommended temporal dimension name is "t" - recommended bands dimension name is "bands" other refs: Open-EO#123, Open-EO#132, Open-EO#128
Fixed GEE example for the 1.0.0rc2 version of it.
Made some changes in the client to be compliant with 1.0.0rc2.
Added the possibility to apply complex apply process graphs.
Implemented multiple file result download.