openscm / scmdata

Handling of Simple Climate Model data
https://scmdata.readthedocs.io
BSD 3-Clause "New" or "Revised" License
8 stars 5 forks source link

Type information for RunGroupBy.apply() #258

Open mikapfl opened 1 year ago

mikapfl commented 1 year ago

Is your feature request related to a problem? Please describe.

I get the error:

error: Call to untyped function "apply" in typed context  [no-untyped-call]

if I use run.groupby(…).apply(…). That's a bit sad.

Describe the solution you'd like

It would be great to add type information to RunGroupBy.apply(), however that's not really possible because there would be a dependency cycle: run.groupby() returns a RunGroupBy object (and therefore has to import scmdata.groupby), and RunGroupBy.apply() return an ScmRun object (and therefore has to import scmdata.run).

I'm not sure how to properly solve this.

Describe alternatives you've considered

We could:

znichollscr commented 1 year ago

Is it not possible to use from __future__ import annotations combined with if TYPE_CHECKING to get around this?

mikapfl commented 1 year ago

Hm, maybe it is. I think I tried using quotes around types (the equivalent of from __future__ import annotations), but I didn't try if TYPE_CHECKING, so maybe the combination can do the trick.

lewisjared commented 1 year ago

I think the TYPE_CHECKING guard should work.

I don't think `RunGroupBy is well-typed currently either.

RunGroupBy could use a generic with a TypeVar bound to a BaseScmRun (I think I've used the poorly named T variable). Then the type on run.groupby would be RunGroupBy[Self] and the apply functions would know which type they should return.

mikapfl commented 1 year ago

The generic TypeVar could work, but binding it to a BaseScmRun would introduce dependency circles again, no? We could just not bind it at all, which I think would be fine, no one is instantiating RunGroupBys out of whole cloth anyway.

lewisjared commented 1 year ago

Added typehints in #262