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

Change product finding to handle lists of products #1201

Merged
merged 2 commits into from Sep 29, 2021

Conversation

alexgleith
Copy link
Contributor

@alexgleith alexgleith commented Sep 28, 2021

Reason for this pull request

Searching for datasets by including a list of products is possible, this makes it accessible using the normal query method.

Proposed changes

@codecov
Copy link

codecov bot commented Sep 28, 2021

Codecov Report

Merging #1201 (7f4ee44) into develop (d268f49) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1201   +/-   ##
========================================
  Coverage    93.79%   93.79%           
========================================
  Files          102      102           
  Lines        10403    10406    +3     
========================================
+ Hits          9757     9760    +3     
  Misses         646      646           
Impacted Files Coverage Δ
datacube/api/query.py 94.78% <100.00%> (-0.03%) ⬇️
datacube/index/_products.py 94.48% <100.00%> (+0.15%) ⬆️

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 d268f49...7f4ee44. Read the comment docs.

Copy link
Contributor

@jeremyh jeremyh left a comment

Choose a reason for hiding this comment

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

Tests added / passed. NOT A GOOD TEST. ANY IDEAS ON A BETTER ONE?

Yeah, I'm wary of Mock tests. It's too easy for them to miss the actual useful parts to test (and be tied to internals, so break easily).

The integration_tests folder contains fixtures that create real indexes to test/query against.

For example, something like this should reproduce it, using the fixtures inside (integration_tests/index/test_search.py):

def test_query_dataset_multi_product(index: Index,  ls5_dataset_w_children: Dataset):
    # We have one ls5 level1 and its child nbar
    dc = Datacube(index)

    # Can we query a single product name?
    datasets = dc.find_datasets(product='ls5_nbar_scene')
    assert len(datasets) == 1

    # Can we query multiple products?
    datasets = dc.find_datasets(product=['ls5_nbar_scene', 'ls5_level1_scene'])
    assert len(datasets) == 2

Copy link
Contributor

@SpacemanPaul SpacemanPaul left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@SpacemanPaul
Copy link
Contributor

Oh, and does this need an entry in whats_new?

@alexgleith
Copy link
Contributor Author

Wait, I meant to push another commit. Hold on.

@alexgleith
Copy link
Contributor Author

I don't think it needs to go in what's new, @SpacemanPaul as it doesn't actually change anything... and it's an internal API fix.

@Kirill888
Copy link
Member

I think this is good enough fix for right now, but a "proper" fix might require handling the case where product is not supplied at all, but implied via other parameters of the query. But given that almost no-one uses that mode of operation and I don't even fully understand what Query class is doing with the product (removes some search terms from the query if some products are 3d?) there is certainly no rush to implement that.

@Kirill888
Copy link
Member

Kirill888 commented Sep 29, 2021

Can we also handle the case when multiple products are supplied as a tuple?

def _listify(v):
return v if isinstance(v, list) else [v]

will require fixing code above

def _listify(v):
    return [v] if isinstance(v, str) else [*v]

@alexgleith
Copy link
Contributor Author

I handled it differently. Thoughts @Kirill888 ?

@Kirill888
Copy link
Member

@alexgleith I don't see it being handled? I'm talking about supplying dc.load(product=("a", "b")) instead of dc.load(product=["a", "b"]), code you changed will handle both cases, but tuple version will break inside code that hasn't been touched by this PR

@alexgleith
Copy link
Contributor Author

Ok, tests passing and added the tuple handling @Kirill888.

Bump @SpacemanPaul for a re-review please?

@alexgleith alexgleith merged commit 8a9f93f into develop Sep 29, 2021
@alexgleith alexgleith deleted the fix-query-search-product branch September 29, 2021 02:30
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

4 participants