Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Getting COOPS stations within region #60

Closed
SorooshMani-NOAA opened this issue Feb 27, 2023 · 3 comments · Fixed by #92
Closed

Getting COOPS stations within region #60

SorooshMani-NOAA opened this issue Feb 27, 2023 · 3 comments · Fixed by #92
Assignees

Comments

@SorooshMani-NOAA
Copy link
Contributor

In stormevents there's this assumption in one of the automated tests that if an empty geometry is passed to the coops_stations_within_region, the return data frame will be empty. Right now that test is broken because the logic in searvey is that:

searvey/searvey/coops.py

Lines 824 to 827 in d654ad6

if region:
return stations[stations.within(region)]
else:
return stations

@pmav99, @carolakaiser in your opinions, does it make sense to keep searvey as it is and just update the stormevents test or should I modify how coops_stations_within_region works in searvey?

What was the use-case for adding this code @carolakaiser? Do you have a case where if you pass empty geometry then you want to get all of the stations? Or are you passing None instead of empty geometry to get all the stations? If you're usecase is covered by passing None, then we can change this to if region is None: then both of our use-cases are covered.

What do you think?

@pmav99
Copy link
Member

pmav99 commented Feb 27, 2023

If region is None is the proper fix here. I.e. if the user did not specify a specific region, return the full dataset.

I would probably write it like this though:

if region is not None:
    stations = stations[stations.within(region)]

return stations

@SorooshMani-NOAA SorooshMani-NOAA self-assigned this Feb 28, 2023
@SorooshMani-NOAA
Copy link
Contributor Author

Thank you, I'll also wait for @carolakaiser before making this change, just to make sure I don't break her workflow either.

@carolakaiser
Copy link
Contributor

if region is None sounds like a good approach. I think this would work well.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants