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

Protocols #1212

Closed
IvarStefansson opened this issue Aug 9, 2024 · 11 comments
Closed

Protocols #1212

IvarStefansson opened this issue Aug 9, 2024 · 11 comments
Assignees
Labels
user group Issue to be worked on in the internal user group.

Comments

@IvarStefansson
Copy link
Contributor

See #1193.
When completed, delete models_glossary.py.

@IvarStefansson IvarStefansson added the user group Issue to be worked on in the internal user group. label Aug 9, 2024
@Yuriyzabegaev Yuriyzabegaev self-assigned this Aug 21, 2024
@jwboth
Copy link
Contributor

jwboth commented Aug 22, 2024

Task: Explore the possibility to introduce protocols as presented in the issue description. Note, this will may be a highly experimental project and communication is key. The main question to be investigated is what the impacts of using protocols would be on typing and mypy. This can be investigated on small toy classes, or in the realm of PorePy. It may be, it will be challenging to choose a good place to start to introduce a protocol for a test. Model classes like on single phase flow and contact mechanics can be an interesting place to analyze code overlaps and potential gains.

Main outcome: Better feeling how Protocols in practice would interact with typing and mypy, in general and in the context of PorePy. If the outcome is positive, is it possible to give advice how to continue the process.

@Yuriyzabegaev
Copy link
Contributor

Follow-up after the discussion. The branch is here.

  1. The protocol syntax does not fulfill our goals. We have to use the inheritance from a "protocol" base class.
  2. What happens if someone instantiate a model while forgetting to override all the "protocol" declarations - runtime error. Solutions: a) raise NotImplementedError b) abstract base class c) conditional definition via IS_TYPE_CHECKING.
  3. Docstrings move to the protocol. What about the methods, such as permeability, that have multiple docstrings in the overrides?
  4. We lose the locality of dependencies, e.g. this mixin relies only on this and this method. Is this a bad thing?
  5. How to separate the protocols for the physical models?

@jwboth
Copy link
Contributor

jwboth commented Aug 28, 2024

Some thoughts - I do not claim they are correct. I am still not super happy with that everything (at the moment) is a PorePyModel protocol. It is indeed true that (in the way we set up models with mixins) it will be part of a PorePyModel. But this assumption is not made explicit in the code design. The safety cannot be assured. As a user I should be easily be able to break this assumption - this is nothing new, still.

I think protocols still provide the opportunity to use them as interfaces (when using abstract base classes, of course, it is no protocol anymore). And, e.g., constitutive laws, e.g., FouriersLaw could be of type GridFunction (or so), where GridFunction is a protocol ensuring access to geometry/grid related operators. This could more explicitly indicate what access to allow - after all, the main usage of protocols outside of PorePy is their possibility in captilizing on mypy to be a developing tool and not in order to make mypy silent.

Once, one adds subtyping of that sort, one could also consider polymorphism and add further subtypes, e.g., ThermalFunction, where ThermalFunction ensures access to methods inherent to thermal scenarios; temperature for instance.

With such a more modular subtyping, one could identify methods like EnthalpyFromTemperature of different type.

I do not claim that one can easily continue and introduce logical subtypes. But I strongly believe, it would be worth to try to make the effort.

@keileg
Copy link
Contributor

keileg commented Aug 28, 2024

These are interesting thoughts that seem worthy of further consideration, @jwboth. Do you think this can be explored in the context of considering a few cases of specific physics?

@jwboth
Copy link
Contributor

jwboth commented Aug 28, 2024

I believe one can apply similar thoughts on the physics-level. After a quick glance at MomentumBalance and FluidMassBalance, my intuition would be try to identify an overlap, which characterizes a general protocol for something like a GridBasedEquation (equation system, grid qtys (see GridFunction)) in addition to a physics-specific (hydrodynamic and elastic) protocol. It is not trivial.

@Yuriyzabegaev
Copy link
Contributor

I wrote the sketch of the protocols for the fluid mass balance. I tried to separate some logic into separate protocols according to @jwboth.

@jwboth
Copy link
Contributor

jwboth commented Sep 5, 2024

Great job so far! It is time for a draft PR. It would be great if you could be careful in the PR description aiming at providing insight into your design choices. Here, it would be also good to be consistent in the naming of protocols (yet details can be also fixed later in the PR review).

Another next step would be a reiteration of the constitutive laws with the aim to be slightly more modular and descriptive instead of inheriting only from PorePyModel. I believe your recent changes on the modular protocol design for the single physics could enter. Please check out #1089 . To me classifying the constitutive laws in terms of protocols is tightly connected to adding some structure as intended in this issue. We do not need to split the file for now. But maybe we could go a step towards and think of appropriate protocols. Up for discussion!

@mariusnevland mariusnevland mentioned this issue Sep 5, 2024
13 tasks
@keileg
Copy link
Contributor

keileg commented Sep 12, 2024

Summary after discussions in person: We will limit our ambitions in terms of modularity and go for one protocol per physics model, with use of inheritance in the case of multiphysics models. Methods and attributes that do not naturally fit into any of these protocols will initially be gathered in a single, 'everything else' protocol. This may be split later, depending on what it ends up containing.

@Yuriyzabegaev Yuriyzabegaev mentioned this issue Oct 3, 2024
13 tasks
@vlipovac
Copy link
Contributor

vlipovac commented Oct 4, 2024

Hello everyone,

after opening this can of worms, as @keileg put it, I had to check up on this issue, especially about the problem of inheritance highlighted by @Yuriyzabegaev in #1234.

The fundamental issue is to have attributes annotated, without messing with the MRO, which is critical in this mixin setting.
Inheritance is unavoidable in some form, since it is key for PyLance/Intellisense to work (be able to navigate through the code using static type checkers).

The solution I found is to shift the inheritance onto the metalevel.
By Introducing intermediate Metalevel classes, which do nothing but inherit and pass on the type hints in protocols on the class-level, the problem highlighted in PR #1234 should be resolved.

The following code is a working example of @Yuriyzabegaev 's example, where mypy does not complain.
If you copy it into your editors, you should be able to navigate to the declarations of a and b in the protocol, at any level of the hierarchy.

from typing import Protocol, Callable, Protocol

import numpy as np
import porepy as pp


class SinglePhaseFlowProtocol(Protocol):

    a: Callable[[], np.ndarray]

class MomentumBalanceProtocol(Protocol):

    b: Callable[[], pp.ad.Operator]


class MetaMBP(MomentumBalanceProtocol, type):
    pass

class MetaSPFP(SinglePhaseFlowProtocol, type):
    pass

class LinearMechanicsElasticity(
    metaclass=MetaMBP,
):

    def test_lme(self):
        out = self.b()
        assert isinstance(out, pp.ad.Operator)
    

class MixedProtocol(MetaMBP, MetaSPFP, type):
    ...


class PressureStress(
    LinearMechanicsElasticity,
    metaclass=MixedProtocol,
):

    def test_ps(self):
        self.test_lme()
        outa = self.a()
        outb = self.b()

        assert isinstance(outa, np.ndarray)
        assert isinstance(outb, pp.ad.Operator)

class PoroMechanicsPorosity(metaclass=MixedProtocol):

    def test_pmp(self):
        outa = self.a()
        outb = self.b()

        assert isinstance(outa, np.ndarray)
        assert isinstance(outb, pp.ad.Operator)


class ConstitutiveLawsPoromechanics(PressureStress, PoroMechanicsPorosity):
    
    def test_clp(self):
        outa = self.a()
        outb = self.b()
        self.test_lme()
        self.test_ps()
        self.test_pmp()

        assert isinstance(outa, np.ndarray)
        assert isinstance(outb, pp.ad.Operator)


class MyMixin:

    def a(self):
        print('a')
        return np.ones(3)

    def b(self):
        print('b')
        return pp.ad.Operator('dummy')


class MyModel(MyMixin, ConstitutiveLawsPoromechanics):
    pass

model = MyModel()
# This will print the correct sequence a-b-b-b-a-b-a-b, according to how the test
# functions are nested
# Also, the mixed-in definition of a() and b() works (see above assert statements)
model.test_clp()

Why does this work?

The inheritance on metalevel works solely as an inheritance of class members. (methods of a metaclass are automatically classmethods of instances of classes, which use the metaclass).
And there is no method resolution order on this level.

The only restriction we have is, that mixed protocols have to be put on a meta level, such that the mixed meta class inherits the meta-versions of individual base protocols.

Still to investigate

We have to check which of the two protocol approaches are admissible:

def foo(grid: pp.GridLike) -> Any:
    pass

or
foo: Callable[[pp.GridLike], Any]

Both approaches allow docstrings for intra-sphinx linking and docstrings do not mess with the code.
But I suggest the latter one, as it is a pure declaration without actually putting any member in the memory (we shift all the worry away from the protocols to the actual implementation).

Also, I think the role of the actual intended use of protocol at this level should be discussed

@IvarStefansson
Copy link
Contributor Author

Do you agree we can close this, @keileg?

@keileg
Copy link
Contributor

keileg commented Jan 3, 2025

Agree, let's put this to rest (for now).

@keileg keileg closed this as completed Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user group Issue to be worked on in the internal user group.
Projects
None yet
Development

No branches or pull requests

6 participants