docs: Fix documentation #409
Conversation
With this change, the examples will be executed in the documentation
…sor.plot_by_bands and ImageAccessor.plot_by_regions methods
…t show information
…ection "LANDSAT/LC08/C01/T1_TOA"
In the past, it was called with the name "indices"
…maskClouds Change the example into one more fastest, update COPERNICUS/S2_SR to COPERNICUS/S2_SR_HARMONIZED (the first is deprecated) and add a minor todo note to improve an example
In that order it shows in order RGB (instead of BGR)
For the performance of making documentation, I left the examples as code-block as before. Maybe, in other opportunity, they will be a jupyter executable as before
|
I executed |
|
Also, I realised about two things:
def foo(param: list = []): ...Doing this is dangerous. It is preferible to do something like: def foo(param: list = None):
param = param if param is not None else []
...that is safer.
Do you want to include this changes in this PR? |
|
Also, there are many parameters outdated in many classes. For example, in the method |
|
I run the documentation and it looks really nice. Please give my any comment if you prefer something different. I think the PR is ready to be reviewed. |
There was a problem hiding this comment.
Some methods uses mutable parameters, for example
We realized that wih Rodrigo some time ago, I would suggest to do it in a follow-up PR
PEP 8 recommends a limit of 72 characters for docstring. It could be quite restrictful for us, but maybe a limitation to 80 or 88 characters it would be fine.
72 character is way to small, we have very long docstring with lots of comments and explainations I would stick to what it is now and make good use of our IDE wrapper. If at some point we really feel it's slowing us down or making the lib complex to use I'll consider changing it.
Also, there are many parameters outdated in many classes.
I kept them to be consistent with the old implementation. If we want to remove them we need to use a deprecation cycle to avoid breaking older code bases and that should definitely be done in another PR. For the one you highlighted that was done to respect the older version of the geojson representation that could include crs but it was dicarded later down the road and now everything is always in 4326.
|
The tests should not be failing considering your modifications. I run them several times on my end and it works perfectly fine, it's as if it was unable to load the json file anymore. I'll merge it and it the bug persist, I'll regenerate a key from my google console. |
I realised that many methods have some bugs in their documentation. For instance,
getOffsetParamshas an example which is not well idented.This PR aims to fix these errors in the documentation.
The changes are:
ee.ImageCollection(‘COPERNICUS/S2_SR’).first().getSTAC()toee.ImageCollection(‘COPERNICUS/S2_SR’).first().geetools.getSTAC()).getScaleParamsreturns adict, instead ofdict[str, float]that is the complete typing, or update the wrong typingee.imagetoee.Image).