OpenDrift / opendrift

Open source framework for ocean trajectory modelling
https://opendrift.github.io
GNU General Public License v2.0
247 stars 120 forks source link

`covers_positions` routine with diverging return types #1305

Open poplarShift opened 4 months ago

poplarShift commented 4 months ago

https://github.com/OpenDrift/opendrift/blob/857fb75a598f186993aa93b6edb62acf1526885e/opendrift/readers/operators/readerops.py#L46 returns an array

whereas these, and all the other ones I can find, return a boolean: https://github.com/OpenDrift/opendrift/blob/857fb75a598f186993aa93b6edb62acf1526885e/opendrift/readers/basereader/unstructured.py#L134 https://github.com/OpenDrift/opendrift/blob/857fb75a598f186993aa93b6edb62acf1526885e/opendrift/readers/interpolation/structured.py#L143

what is the reasoning behind that?

gauteh commented 4 months ago

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 commented 4 months ago

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

gauteh commented 4 months ago

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

knutfrode commented 4 months ago

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 commented 4 months ago

Yes, that sounds good.

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

poplarShift commented 3 months ago

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