Access to read_shape.__on_land__ from outside of class causes regression error

[lencart]

Hi @knutfrode, @gauteh ,

I'm a proposer of yet-to-be integrated feature that changes reader_shape.py (PR #1460).
This implementation stayed away from public methods but it changed the signature of reader_shape.Reader.__on_land__().
Tests were added to test_reader_shape.py and all tests passed after the latests changes from the main opendrift branch.

However, the reader crashes when the basemodel.__init__() tries to access __on_land__() directly.
This change was introduced by @poplarShift on a691efa (line 934) replacing the use of reader_shape.Reader.is_inside().

My approach was to change a private method's signature instead of creating another method behind it, to avoid clutter. I felt confident to only write unit tests for local client code since I assumed __on_land__() to be a non-public method do to the underscores. I now see this was wrong to assume this since dunder notation does not signal non-public methods necessarily.

I can change back the signature and implement my changes differently but I see a set of issues here:

1. If __on_land__ is to be a non-public method (there are no private methods in python) then it shouldn't be accessed from outside its class. If there is need for that functionality, that needs to be in a public method (no underscores).
2. The use of double underscores "dunder" methods is reserved for python magic functions. We shouldn't implement those. If indeed the method needs to be non-public, choose between a weak non-public that still can be used outside (though highly discouraged) using a single leading underscore (e.g. _weak_non_public_method()) or a strong non-public with a leading double underscore (e.g. __strong_non_public_method()) that will get name-mangled making it impossible to use it directly outside of the class.

Opendrift's codebase is extensive and has already a lot of complex links. Please adopt the intended use of underscores in python to signal methods that will not be called from outside the class so that developers are confident to change classes internally without interfering with their interfaces.

Cheers.

João.

[gauteh]

Hi,

the use double-underscores means that the method is not part of the public API, but it can of course be used within opendrift. As you mention it should probably not end with double-underscores, that was just to make it look prettier.

-- gaute

[lencart]

Hi,

While python lets you adopt your own contract on how to name and structure code, the intended use of leading underscores are to signal internal methods PEP8.
In the context of OOP and encapsulation, this has a different meaning from defining an application-wide API.

To maintain encapsulation in OOP, internal methods should stay internal to the class, behind the class's interface. External methods can be accessed outside of the class. When a method is signalled as "internal" you are saying to a developer using the class to write code in another class (not only to the full software package API external user) that such a method controls the internal state of the object and shouldn't be messed with. This allows for the reduction of bugs when developing inside a given class.

The design of an external user facing API is a different thing. You can "hide" your app-wide internal classes in deeper modules/classes and create public interface classes, importing the "allowed" methods inside __init__ and / or specifying allowed classes on __all__.

In this particular context, I don't understand the changes made to basemodel.__ini__.closest_ocean_points and don't need to, but the creation of the public method reader_shape.Reader.is_inside() seemed to be preferable in terms of unit testing and avoiding regression errors when merging features into a specific class, in this case reader_shape.Reader.

[knutfrode]

These are all very valid points, and Gaute and I fully agree.
It is the intention to follow PEP8, but many methods across the whole OpenDrift codebase should have had a leading underscore to indicate internal use only.
Fixing this has been on the TODO list for a while already, but the shapereader and this particular issue can be a good place to start.

For the readers, the public API was from the beginning supposed to consist of two methods only:

• __init__() The constructor, normally taking a filename or URL as input
• get_variables() The method to return requested variables covering requested times/positions

The shape reader, as well as reader_global_landmask, both have an internal method __on_land__() as the main helper method for get_variables()
It could in principle be made public (on_land() or is_inside()), but we suggest it is better that it remains internal, but with proper name _on_land().

Thus from the outside (e.g. inside basemodel/__init__), the generic/common method get_variables() should be used instead:
landmask = reader.get_variables('land_binary_mask', x=x, y=y)['land_binary_mask']
Slightly more verbose, but more consistent.

I will proceed to implement this, and assume that your new functionality could be adapted to this?
But your comment is welcome.

[lencart]

Great.
That suits my implementation of __on_land__().

[knutfrode]

This should be a first step in the right direction: a3535d3

[poplarShift]

Hi, for the record, the ìs_inside method was part of the same PR so hence never really in the main branch; I deleted it again to avoid using shapely and geopandas. I should have raised the issue of private vs public methods already then, so happy to see the commit referenced above by Knut Frode which definitely solves that.

My only quibble is that a method like _on_land is more intuitive to use than something like .get_variables('land_binary_mask', x=lon_array,y=lat_array)['land_binary_mask'], hence the code is prone to become abused like I did in that PR which used __on_land__ in reader_shape to begin with.