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

covers_positions routine with diverging return types #1305

Open
poplarShift opened this issue May 28, 2024 · 6 comments
Open

covers_positions routine with diverging return types #1305

poplarShift opened this issue May 28, 2024 · 6 comments

Comments

@poplarShift
Copy link
Contributor

def covers_positions(self, lon, lat):

returns an array

whereas these, and all the other ones I can find, return a boolean:

def covers_positions(self, x, y, z=0):

def covers_positions(self, x, y, z=None):

what is the reasoning behind that?

@gauteh
Copy link
Member

gauteh commented May 28, 2024

That seems to be a mistake. The latter two are probably the correct ones, but I am a bit surprised they don't return arrays of booleans.

@knutfrode
Copy link
Collaborator

I am not sure if there is a real reason for this diverging practice. I will have a look.

@gauteh
Copy link
Member

gauteh commented May 28, 2024

I don't think the one in readerops have ever been used in practice before.

@knutfrode
Copy link
Collaborator

The methods actually belong to two different classes:
readers/interpolation/structured.py is asubclass of ReaderBlock whereas the others are subclasses of BaseReader

But anyway, I agree with Gaute that it sounds more logical for all of these methods to return an array of booleans.
Then we could have a simple wrapper onliner method covers_all_positions the simply returns True of all booleans are True.

If you agree, I could make this change consistently.

@gauteh
Copy link
Member

gauteh commented May 30, 2024

Yes, that sounds good.

For ReaderBlock it should have a different name: covers_all_positions, while for the readers it should return indices?

@poplarShift
Copy link
Contributor Author

Thanks! Does this have any bearing on my latest comment #1279 about the possible return of a zero size array?

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

No branches or pull requests

3 participants