-
Notifications
You must be signed in to change notification settings - Fork 147
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
i.eodag: add operators to queryables and allow multiple values #1142
Conversation
|
@HamedElgizery I extensively modified the manual page to demonstrate the usual writing style. Please, have a look to check that I got things right (i.e., search products intersecting the computational region should be the default). I removed the -e flag in one example, as that flag is no longer there. Also, I believe some other options might be explained in the description section. And, as far as I recall from eodag itself, intersects is the default, so |
|
I looked at the code and I don't see big problems. I tested the pull request and most of the command I tried it works. I have a problem trying to query using the id, when I use the id parameter it return an empty result. It could be a eodag problem and not related to the GRASS implementation... |
|
I made an initial test, great work! A few observations: Error to be caught when input vector map not present (just try with a non-existing one): GRASS latlong_wgs84/testarea:~ > i.eodag -l start=2017-10-21 end=2017-10-21 producttype=S2_MSI_L2A provider=cop_dataspace map=non_existing_bbox_map
WARNING: Experimental Version...
WARNING: This module is still under development, and its behaviour is not
guaranteed to be reliable
ERROR: Vector map <non_existing_bbox_map> not found
Traceback (most recent call last):
File "/home/mneteler/software/grass84/dist.x86_64-pc-linux-gnu/scripts/i.eodag", line 1144, in <module>
sys.exit(main())
^^^^^^
File "/home/mneteler/software/grass84/dist.x86_64-pc-linux-gnu/scripts/i.eodag", line 1057, in main
geometry = get_aoi(options["map"])
^^^^^^^^^^^^^^^^^^^^^^^
File "/home/mneteler/software/grass84/dist.x86_64-pc-linux-gnu/scripts/i.eodag", line 296, in get_aoi
if gs.vector_info_topo(vector)["areas"] <= 0:
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/mneteler/software/grass84/dist.x86_64-pc-linux-gnu/etc/python/grass/script/vector.py", line 182, in vector_info_topo
s = read_command("v.info", flags="t", layer=layer, map=map, env=env)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/mneteler/software/grass84/dist.x86_64-pc-linux-gnu/etc/python/grass/script/core.py", line 554, in read_command
return handle_errors(returncode, stdout, args, kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/mneteler/software/grass84/dist.x86_64-pc-linux-gnu/etc/python/grass/script/core.py", line 366, in handle_errors
raise CalledModuleError(module=module, code=code, returncode=returncode)
grass.exceptions.CalledModuleError: Module run `v.info -t layer=1 map=non_existing_bbox_map` ended with an error.
The subprocess ended with a non-zero return code: 1. See errors above the traceback or in the error output.And it would be good to have a default cloud value defined and visible to the user as it is unclear from the description, to which threshold And: which is the default
Indeed I found the downloaded S2 data in Thanks again for your work, my first |
src/imagery/i.eodag/i.eodag.html
Outdated
| <em>i.eodag</em> reads user credentials from the EODAG YAML config file. | ||
| Users have to specify the config file path through the <b>config</b> | ||
| option, otherwise <em>i.eodag</em> will use the credentials found in | ||
| the default config file <em>~/.config/eodag/eodag.yml</em> |
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.
| the default config file <em>~/.config/eodag/eodag.yml</em> | |
| the default config file <em>~/.config/eodag/eodag.yml</em> which is auto-generated the | |
| first time eodag is used after an install in a local directory (`$HOME/.config/eodag/eodag.yml` | |
| on Linux and BSD; XXXX on Windows; YYYY on Mac). |
I suggest to add a note where the config is... (dunno where it would be on Windows or Mac).
Yet it looks like a chicken-egg problem: pip install eodag will install the software but what will generate the config? At first run on i.eodag if we do not want to suggest to run the eodag CLI tool? Perhaps I overlook something.
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 suggest to add a note where the config is... (dunno where it would be on Windows or Mac).
I tested it on Mac, and it would be in the same location $HOME/.config/eodag. Don't know about Windows though, yet.
Yet it looks like a chicken-egg problem:
pip install eodagwill install the software but what will generate the config? At first run oni.eodagif we do not want to suggest to run theeodagCLI tool? Perhaps I overlook something.
Maybe we could add a flag i, for initialization, to use when running i.eodag for the first time. Just for the sake of making it less confusing, and then maybe we could use that flag to write a custom GRASS GIS EODAG config file, if we need to.
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't that just happen within i.eodag (with a check or so) without another flag? Users should be able to create their config files or modify the default one.
At this point, we need to move forward with the other objectives of the GSoC project.
Thanks, @veroandreo. It is more consistent and clear now. I have gone through it and everything looks right. I only added some
Just added that. |
To filter by the id you need to use the queryable Or... |
option Faulty filtering would take place without user permission, when searching with ids. This commit fixes that.
64858d9
to
44150b7
Compare
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 do have some comments, but they are not urgent. So, I think we can merge.
As mentioned, tests would be nice, ideally based on the examples you added...
|
I will merge this one once tests finish. The remaining "questions" were moved here: HamedElgizery#17 |
I reverted this change (setting area_relation to Intersects as a default value), and now it has no default value. The reason is that the default value for area_relation makes i.eodag do unintended filtering for products passed from a GeoJSON file. If filtering of products from a GeoJSON file with respect to the area_relation is needed, then area_relation needs to be specified explicitly. It was reverted here 44150b7. |
This PR fixes the current faulty query option, and add the ability to use operators (eq, lt, gt, le, etc...).
It also allows the query option to have multiple queryables (emulating the AND relation), and allows each queryable to have multiple value (emulating the OR relation).
Querying section is also added to the manual, with examples on how to use the query option.
Multiple use cases for i.eodag is also added to the manual.