-
Notifications
You must be signed in to change notification settings - Fork 89
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
Comments
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. |
Follow-up after the discussion. The branch is here.
|
Some thoughts - I do not claim they are correct. I am still not super happy with that everything (at the moment) is a 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., Once, one adds subtyping of that sort, one could also consider polymorphism and add further subtypes, e.g., With such a more modular subtyping, one could identify methods like 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. |
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? |
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. |
I wrote the sketch of the protocols for the fluid mass balance. I tried to separate some logic into separate protocols according to @jwboth. |
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 |
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. |
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. The solution I found is to shift the inheritance onto the metalevel. The following code is a working example of @Yuriyzabegaev 's example, where mypy does not complain. 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). 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 investigateWe have to check which of the two protocol approaches are admissible: def foo(grid: pp.GridLike) -> Any:
pass or Both approaches allow docstrings for intra-sphinx linking and docstrings do not mess with the code. Also, I think the role of the actual intended use of protocol at this level should be discussed |
Do you agree we can close this, @keileg? |
Agree, let's put this to rest (for now). |
See #1193.
When completed, delete models_glossary.py.
The text was updated successfully, but these errors were encountered: