-
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
Remove the invalid default CRS #233
Conversation
b4309bd
to
faae692
Compare
faae692
to
7407af6
Compare
Getting rid of invalid But I'm not sure we should remove it completely instead of replacing it with valid int |
I've added a non 4326 example to the filter_bbox documentation. If all examples use 4326 and we want some non-default examples, we should probably replace some with non 4326 bboxes as examples instead. |
To simplify review, I already cherry-picked a large part of the original PR in 75e4a58 |
@@ -131,7 +131,7 @@ pixels to 0 and all other pixels to 1 using a simple comparison:: | |||
s2_sceneclassification = ( | |||
connection.load_collection("TERRASCOPE_S2_TOC_V2", bands=["SCENECLASSIFICATION_20M"]) | |||
.filter_temporal(extent=["2016-01-01", "2016-03-10"]) | |||
.filter_bbox(west=5.1518, east=5.1533,south=51.1819,north=51.1846, crs=4326) | |||
.filter_bbox(west=5.1518, east=5.1533,south=51.1819,north=51.1846) |
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 keep these two cases of crs 4326
because of its documentation value
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 guess it would make sense to describe somewhere that 4326 is the default and can be omitted - or use another CRS for the example.
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.
yeah that line (and other lines) "need work" anyway: you have to scroll horizontally to see everything on https://open-eo.github.io/openeo-python-client/basics.html#example-applying-a-mask
} | ||
if crs is not None: | ||
extent.update(crs=crs) |
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.
There should be a bit of test coverage of this (and on datacube.py too)
fyi: I'll need to get back to this after my vacation, so this will stay open and unresolved for about 3 weeks. |
No problem, I kept some things open because I wanted to improve unit test coverage of them first |
The remaining bits are now also incorporated in (1fe248f) (closes this PR) |
Remove
crs = "EPSG:4326"
from examples due to the following reasons:4326
) is the default CRS anyway, so it is redundant.Also, the Python client should not send the crs as null.
I have not updated the Notebooks.