Fix proj extension regression.#253
Conversation
SpacemanPaul
force-pushed
the
item-proj-fix
branch
from
4e71fc7 to
ac02908
Compare
|
🚀 Deployed on https://f6d43600145741f0--odc-stac-docs.netlify.app |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #253 +/- ##
===========================================
+ Coverage 89.56% 89.60% +0.03%
===========================================
Files 9 9
Lines 1380 1385 +5
===========================================
+ Hits 1236 1241 +5
Misses 144 144 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SpacemanPaul
force-pushed
the
item-proj-fix
branch
from
ac02908 to
9009b42
Compare
| and data_assets | ||
| and not any(has_proj_data(a) for a in data_assets.values()) | ||
| ): | ||
| has_proj = False |
There was a problem hiding this comment.
Should has_proj still be True if there are no data_assets?
There was a problem hiding this comment.
Does it matter?
There was a problem hiding this comment.
I'm really not sure, but the mismatch between having has_proj = True for an item with the proj ext and no assets but has_proj = False for an item with the proj extension but no proj data in the assets struck me as a little odd.
There was a problem hiding this comment.
See how has_proj is used in _parse_item() below in this file:
has_proj = False if template.has_proj is False else has_proj_ext(item)
expects that template.has_proj (set by the code here) can be different.
There was a problem hiding this comment.
And we need to set it to False so the band2grid method gets called by _parse_item.
SpacemanPaul
dismissed
Ariana-B’s stale review
AS discussed, this PR needs more attention.
SpacemanPaul
force-pushed
the
item-proj-fix
branch
from
2d61193 to
ea26760
Compare
SpacemanPaul
force-pushed
the
item-proj-fix
branch
from
ea26760 to
253ecbe
Compare
SpacemanPaul
force-pushed
the
item-proj-fix
branch
from
253ecbe to
5acb824
Compare
| ): | ||
| _, band2grid = compute_eo3_grids(data_assets) | ||
| else: | ||
| band2grid = band2grid_from_gsd(data_assets) |
There was a problem hiding this comment.
The double if-statements with almost the same condition seems convoluted. Shouldn't has_proj = False be in this block instead of the separate if-statement on lines 677-684?
If the value of has_proj is important after having taken this branch, I'm guessing the RHS could be some boolean expression (has_proj = has_proj and ..).
There was a problem hiding this comment.
Oops - IIRC I intended the code at 687-692 to replace the code at 677-684.
(The intent was to keep the logical choice of eo3 grids vs gsd grids, but leave the has_proj value unmodified in RasterCollectionMetadata.)
That explains why I didn't have to modify the test in that commit - I thought that was odd.
But I can see why that idea doesn't work now.
| if len(data_bands) == 0 and check_proj: | ||
| # If no data bands found with check_proj=True, fallback to check_proj=False | ||
| # This handles items that declare proj extension at item level but don't have | ||
| # per-asset proj data (shape/transform) | ||
| data_bands = dicttoolz.itemfilter(lambda kv: _keep(kv, False), item.assets) |
There was a problem hiding this comment.
Is this block still necessary? I thought the new check_proj definition took care of this case.
There was a problem hiding this comment.
I think so? It's the fallback if nothing is found with check_proj=True.
There was a problem hiding this comment.
Your comment says, "This handles items that declare proj extension at item level but don't have per-asset proj data", but check_proj will already be false for those items, so I think it's redundant.
There was a problem hiding this comment.
Hmm... Well removing it breaks the tests, so it's definitely doing something.
3 participants
Closes #251
Based closely on code provided by @lukesdm