search

nsidc / earthaccess

Python Library for NASA Earthdata APIs
https://earthaccess.readthedocs.io/
MIT License
403 stars 80 forks source link

Support multiple-point searches #491

Open mfisher87 opened 6 months ago

mfisher87 commented 6 months ago

https://cmr.earthdata.nasa.gov/search/site/docs/search/api.html#c-point

https://github.com/nsidc/earthaccess/discussions/490

jhkennedy commented 6 months ago

Possibly related to #504

mfisher87 commented 4 months ago

cc @Sherwin-14

Sherwin-14 commented 1 month ago

@mfisher87 Hey, needed some clarity over this. I went through the search_data function and search.py file, the file has a point method implemented already so wouldn't that suffice? If no, what can be the exact steps to support multiple point searches and also the likes of polygon and line as referenced here.

Sherwin-14 commented 1 month ago

Looking at #504 do we need to do a similar task ( improving polygon coordinate handling) as the OP suggested in the discussion?

chuckwondo commented 1 month ago

I suggest opening an enhancement request against python_cmr. That's where all such CMR search enhancement requests should be made first, because the ultimate goal is to remove all duplicate CMR search functionality from earthaccess.

The reason we have duplicate functionality in earthaccess is that we previously were unable to get enhancements made to python_cmr. Now that we have a responsive team on python_cmr, we should start by making CMR search enhancement requests there.

We might still end up duplicating code here until python_cmr reaches parity with the duplicated functionality here, but in order to better facilitate being able to remove the duplicated functionality here, we must be sure to start adding new functionality to python_cmr first, then duplicate it here, as necessary, until we get to the day when we can rip out the duplicate code here.

mfisher87 commented 1 month ago

the file has a point method implemented already so wouldn't that suffice

https://github.com/nsidc/earthaccess/blob/be1ec48a213cd9158c7d6a7542a56477cfc14d3e/earthaccess/search.py#L860-L873

This method only supports a single point currently, but we want to support any number of points to reach parity with the CMR API we're using under the hood. As chuck mentioned, the python_cmr project is now accepting PRs and feature requests :100:

Sherwin-14 commented 1 month ago

That means I might need to open up a feature request in python_cmr repo? I thought I should try there so maybe we can discuss the solution here and then open up a PR there?

chuckwondo commented 1 month ago

the file has a point method implemented already so wouldn't that suffice

This method only supports a single point currently, but we want to support any number of points to reach parity with the CMR API we're using under the hood. As chuck mentioned, the python_cmr project is now accepting PRs and feature requests 💯

Technically, this method supports whatever python_cmr supports, since it simply invokes the superclass method (which is from python_cmr), so if python_cmr were to support multiple points, so would earthaccess.

It turns out that python_cmr currently supports only 1 point, so please open an enhancement request in python_cmr to support multiple points: https://github.com/nasa/python_cmr/blob/develop/cmr/queries.py#L491-L506

mfisher87 commented 1 month ago

Technically, this method supports whatever python_cmr supports, since it simply invokes the superclass method (which is from python_cmr), so if python_cmr were to support multiple points, so would earthaccess.

Maybe I'm missing something, but it looks to me like there's more to this. When python_cmr begins to support two argument forms for backwards compatibility, e.g. lon: FloatLike, lat: FloatLike (current) and points: list[tuple[FloatLike, FloatLike]] (to enable this feature), earthaccess will continue to expect the former form, only capable of passing lon and lat to python_cmr's superclass method. There wouldn't be a way to pass in the new single-argument list-of-points form! We still need to overload the function signature on our end.

chuckwondo commented 1 month ago

Technically, this method supports whatever python_cmr supports, since it simply invokes the superclass method (which is from python_cmr), so if python_cmr were to support multiple points, so would earthaccess.

Maybe I'm missing something, but it looks to me like there's more to this. When python_cmr begins to support two argument forms for backwards compatibility, e.g. lon: FloatLike, lat: FloatLike (current) and points: list[tuple[FloatLike, FloatLike]] (to enable this feature), earthaccess will continue to expect the former form, only capable of passing lon and lat to python_cmr's superclass method. There wouldn't be a way to pass in the new single-argument list-of-points form! We still need to overload the function signature on our end.

No need to do that. You just need to be able to call point once per point and have the method add to a list of points on each call instead of having each call simply set the 1 and only point that it currently keeps track of.

chuckwondo commented 1 month ago

In case you're wondering, yes, there are existing functions that behave in this manner, where you can call them multiple times to tack on additional values instead of overwriting a sole value. For example: https://github.com/nasa/python_cmr/blob/develop/cmr/queries.py#L396

Sherwin-14 commented 1 month ago

I opened up a issue on python_cmr https://github.com/nasa/python_cmr/issues/72

mfisher87 commented 1 month ago

Thanks all for the explanation and for the paperwork! :)

mfisher87 commented 1 month ago

:100: Awesome work getting this feature merged into python-cmr @Sherwin-14 and @chuckwondo! https://github.com/nasa/python_cmr/pull/73