reduce the boilerplate

[JoshCu]

Perhaps more of a discussion than an issue; can some of the methods just raise not implemented by default and drop the abstractmethod decorator that forces subclasses to explicitly implement every function?
For example if the gridded methods didn't require explicit implementation, I wouldn't need all of this.

This might be too far from the standard, could some of the common values also have default implementations? There are probably other examples too but I've left these unchanged from the bmi python example I copied. Could they also be defaults in the base class?

     def get_start_time( self ):    
        return self._start_time

    def get_end_time( self ):
        return self._end_time

[mcflugen]

@JoshCu I was actually thinking I would remove the abstract base class altogether and switch to protocols as @BSchilperoort suggested in #58. That, I think, would solve your first issue. It wouldn't address your second issue, though. For that maybe we could have a library of base classes one could inherit from for building new BMI components that use standard implementations? We've kind of done things like this for our data components that, for example, wrap xarray.Datasets or a geotiffs.

[mdpiper]

Perhaps more of a discussion than an issue

@JoshCu This is a little off-topic, but we've started a new forum, https://forum.csdms.io, and it would great if we could get more discussions going on BMI. Please check it out and sign up if you're interested.

[JoshCu]

@JoshCu I was actually thinking I would remove the abstract base class altogether and switch to protocols as @BSchilperoort suggested in #58. That, I think, would solve your first issue. It wouldn't address your second issue, though. For that maybe we could have a library of base classes one could inherit from for building new BMI components that use standard implementations? We've kind of done things like this for our data components that, for example, wrap _xarray.Dataset_s or a _geotiff_s.

I haven't worked with protocols before but that looks like a great idea! I could see subclasses like scalarbmi and gridbmi being useful for those types of models. For the second issue I can't think of a robust way to deal with it. Those variables could be added to the subclasses along with default getter methods, but I'm not aware of a clean way to make sure that e.g. _start_time is either used by the implementation or removed and replaced. Other than this which seems clunky

def get_start_time( self ):
    if not hasattr(self, '_start_time'):
        raise AttributeError('_start_time not found, either implement method get_start_time or use _start_time to store model start time')
    return self._start_time

Perhaps more of a discussion than an issue

@JoshCu This is a little off-topic, but we've started a new forum, https://forum.csdms.io, and it would great if we could get more discussions going on BMI. Please check it out and sign up if you're interested.

I'll take a look :) Would it be better to move discussion to there or keep it here? I'm more used to github but if the forum has more visibilty I'm happy to use that. I've only interacted with python-bmi but I could open it up to a more general question about multiple default interfaces in other languages if they might also benefit?

[mdpiper]

Would it be better to move discussion to there or keep it here?

I really don't have a good sense for this yet. I like your suggestion of using GitHub for more specific issues/questions and the forum for more general.

[aaraney]

Having done the same thing, @JoshCu, I certainly can sympathize haha. If you want to avoid the boilerplate, why not use some python meta-programming?

from __future__ import annotations
import abc

def raise_not_implemented(cls: type[abc.ABC]):
    def not_implemented(name: str):
        def inner(self, *args, **kwargs):
            raise NotImplementedError(name)
        return inner

    t = type(
        cls.__qualname__,
        (cls,),
        {name: not_implemented(name) for name in cls.__abstractmethods__},
    )
    def wrapper(*args, **kwargs):
        return t(*args, **kwargs)
    return wrapper


class Base(abc.ABC):
    @abc.abstractmethod
    def foo(self) -> str: ...

    @abc.abstractmethod
    def bar(self) -> str: ...


@raise_not_implemented
class Subclass(Base):
    def foo(self) -> str:
        return "horray"

s = Subclass()
s.bar() # NotImplementedError: bar

This isn't perfect, but could be improved upon to avoid undesirable boilerplate with a small amount of code and effort. @JoshCu, thoughts?

[JoshCu]

why not use some python meta-programming?

Truthfully, I've not worked much with python like that and I wasn't entirely sure how to do it, but that looks pretty neat!
It did take me a minute to figure out how it worked though, maybe something a bit less advanced would be easier for people with less python experience to work with? Including myself it would seem!
Although if the alternative is creating a highly nested structure of sub-classes for various model types maybe this would be the simpler way to go.

I'll try it out and report back if I find an alternative worth suggesting.

[hellkite500]

I have been (slowly) working on a python BMI sdk package, and part of that implements an intermediate ABC that does just this, reducing boilerplate and providing some potential default mechanics (which can of course still be overridden).

https://github.com/hellkite500/bmi_pytorch/blob/main/bmi_sdk/bmi_sdk/bmi_minimal.py

[aaraney]

It did take me a minute to figure out how it worked though, maybe something a bit less advanced would be easier for people with less python experience to work with?

@JoshCu, that is totally fair! We are getting into magical spell territory.

This might be a little more approachable?

raise_not_implemented decorator example

from __future__ import annotations

import functools
import sys

import bmipy
import numpy as np

if sys.version_info < (3, 10):
    import typing_extensions as typing
else:
    import typing

_R = typing.TypeVar("_R")
_P = typing.ParamSpec("_P")

# ignore crazy type hints, just makes static tools happy
def raise_not_implemented(
    fn: typing.Callable[_P, _R],
) -> typing.Callable[_P, typing.NoReturn]:
    name = fn.__name__

    @functools.wraps(fn)
    def wrapper(*args: _P.args, **kwargs: _P.kwargs) -> typing.NoReturn:
        raise NotImplementedError(name)

    return wrapper


class BmiBase(bmipy.Bmi):
    # Uniform rectilinear
    @raise_not_implemented
    def get_grid_shape(self, grid: int, shape: np.ndarray) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_spacing(self, grid: int, spacing: np.ndarray) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_origin(self, grid: int, origin: np.ndarray) -> typing.NoReturn: ...
    # Non-uniform rectilinear, curvilinear
    @raise_not_implemented
    def get_grid_x(self, grid: int, x: np.ndarray) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_y(self, grid: int, y: np.ndarray) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_z(self, grid: int, z: np.ndarray) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_node_count(self, grid: int) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_edge_count(self, grid: int) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_face_count(self, grid: int) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_edge_nodes(
        self, grid: int, edge_nodes: np.ndarray
    ) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_face_edges(
        self, grid: int, face_edges: np.ndarray
    ) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_face_nodes(
        self, grid: int, face_nodes: np.ndarray
    ) -> typing.NoReturn: ...
    @raise_not_implemented
    def get_grid_nodes_per_face(
        self, grid: int, nodes_per_face: np.ndarray
    ) -> typing.NoReturn: ...