Skip to content
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

Feat/batch #141

Merged
merged 29 commits into from
Oct 19, 2020
Merged

Feat/batch #141

merged 29 commits into from
Oct 19, 2020

Conversation

AleksMat
Copy link
Contributor

Implements a Python interface for Sentinel Hub Batch Processing API. It also adds tests and a tutorial notebook.

Closes #136

Note: this PR also contains changes from PR #138, which should be merged first.

Copy link
Contributor

@batic batic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First part of review process - going through files on GitHub.

examples/data/california_crop_fields.geojson Show resolved Hide resolved
examples/data/interpolation_evalscript.js Show resolved Hide resolved
sentinelhub/decoding.py Show resolved Hide resolved
sentinelhub/download/client.py Show resolved Hide resolved
sentinelhub/sentinelhub_request.py Show resolved Hide resolved
Copy link
Contributor

@batic batic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I'm ok with SHB, but I'd also like to see methods reordered groups of similar functionality methods (e.g. all properties together, all static methods together, everything dealing with creation of urls together etc.)

sentinelhub/sentinelhub_batch.py Outdated Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Outdated Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Outdated Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
sentinelhub/sentinelhub_batch.py Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Oct 7, 2020

Codecov Report

Merging #141 into develop will decrease coverage by 0.83%.
The diff coverage is 80.48%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #141      +/-   ##
===========================================
- Coverage    90.88%   90.05%   -0.84%     
===========================================
  Files           30       31       +1     
  Lines         3061     3257     +196     
===========================================
+ Hits          2782     2933     +151     
- Misses         279      324      +45     
Impacted Files Coverage Δ
sentinelhub/areas.py 89.62% <26.66%> (-3.71%) ⬇️
sentinelhub/geopedia.py 90.69% <71.42%> (-0.43%) ⬇️
sentinelhub/decoding.py 84.61% <75.00%> (-0.72%) ⬇️
sentinelhub/download/client.py 95.18% <76.47%> (-4.82%) ⬇️
sentinelhub/sentinelhub_batch.py 80.68% <80.68%> (ø)
sentinelhub/sentinelhub_request.py 86.15% <93.33%> (+2.96%) ⬆️
sentinelhub/aws.py 91.66% <100.00%> (+0.05%) ⬆️
sentinelhub/aws_safe.py 98.44% <100.00%> (+0.01%) ⬆️
sentinelhub/constants.py 96.92% <100.00%> (+0.06%) ⬆️
sentinelhub/download/aws_client.py 76.74% <100.00%> (-1.98%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a5b737...68908f4. Read the comment docs.

AleksMat and others added 2 commits October 8, 2020 09:46
@batic
Copy link
Contributor

batic commented Oct 17, 2020

Few more notes on the notebook with the example:

a) descartes package is mentioned to be used, but is not
b) in the "Set up an S3 bucket", I'd also use "warning div" approach (similar as for defining time_interval above), but I leave it up to you to decide.
c) it would be quite useful to put the plot_batch_splitter(splitter) method into BBoxSplitter class and make it a class method. The particular subclass BatchSplitter can then implement it's own plotting (to support tile info). If you agree, please make an issue.

With this, I conclude my long going review (really sorry about that).

@AleksMat
Copy link
Contributor Author

AleksMat commented Oct 19, 2020

a) descartes package is mentioned to be used, but is not

descartes is actually an extra geopandas dependency that is required for making those plots.

b) in the "Set up an S3 bucket", I'd also use "warning div" approach (similar as for defining time_interval above), but I leave it up to you to decide.

For now, I think it is ok to leave this as it is.

c) it would be quite useful to put the plot_batch_splitter(splitter) method into BBoxSplitter class and make it a class method. The particular subclass BatchSplitter can then implement it's own plotting (to support tile info). If you agree, please make an issue.

The problem here is only that I wouldn't want that matplotlib, geopandas, and descartes become sentinelhub-py dependencies. But if we implement this in a way that those are only extra dependecies and one can install the package without them then I agree with this. I'll open a new internal issue about this.

@AleksMat AleksMat merged commit 9f70ba8 into develop Oct 19, 2020
@AleksMat AleksMat deleted the feat/batch branch October 19, 2020 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants