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

rolling_exp: keep_attrs and typing #4592

Merged
merged 3 commits into from
Nov 20, 2020

Conversation

mathause
Copy link
Collaborator

@mathause mathause added topic-internals maintenance topic-metadata Relating to the handling of metadata (i.e. attrs and encoding) labels Nov 18, 2020
Copy link
Collaborator

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mathause ! Excellent tests!

More generally — if keep_attrs doesn't already exist as a kwarg in functions, should we add it? Or just be true?

(tbc, either way having the default be true here is a very welcome feature)

from .dataarray import DataArray # noqa: F401
from .dataset import Dataset # noqa: F401

T_DSorDA = TypeVar("T_DSorDA", "DataArray", "Dataset")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this different from DataWithCoords?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from .common import DataWithCoords
T_DataWithCoords = TypeVar("T_DataWithCoords", bound=DataWithCoords)

works as well. A bit to my surprise as DataWithCoords does not implement reduce. But mypy does not throw an error here:

from xarray.core.common import DataWithCoords
class A:
    pass

hasattr(xr.core.common.DataWithCoords, "reduce") # -> False

def test(x: "A"):
    x.reduce() # mypy errors

def test2(x: "DataWithCoords"):
    x.reduce() # mypy does not error

not sure why not...


(Also I am not entirely sure what the difference is between

T_Parent = TypeVar("T_Parent", bound=Parent)
T_Children = TypeVar("T_Children", ChildA, ChildB)

)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is odd it doesn't throw an error...

On the second case, I think — not completely sure — that T_Parent is generic with an "upper bound" of Parent, so any function that takes T_Parent will return the same type.

Whereas a function that takes T_Children could return either of ChildA or ChildB regardless of what's passed in.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And your call on whether we use DataWithCoords or the TypeVar, but I think if we do the latter we could find a clearer name :)

I would marginally vote to go with DataWithCoords given we do that elsewhere but totally open on trying another approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both would need to be TypeVars. I I'll stick to T_DSorDA as DataWithCoords does not implement reduce. T_DSorDA is already used at some places (map_blocks), e.g.:

T_DSorDA = TypeVar("T_DSorDA", DataArray, Dataset)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I didn't know that, thanks. I wasn't a fan of the name, but we can change them together another time if others agree.

@@ -56,7 +65,7 @@ def _get_center_of_mass(comass, span, halflife, alpha):
return float(comass)


class RollingExp:
class RollingExp(Generic[T_DSorDA]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea with Generic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that took quite a while to figure out but it doesn't work without it as the Type of obj does not bind to the function else. That should come in handy for some other classes.

@mathause
Copy link
Collaborator Author

More generally — if keep_attrs doesn't already exist as a kwarg in functions, should we add it? Or just be true?

I think adding a kwarg does not hurt much but it's probably not used often, either. I don't have a strong opinion.

@max-sixty max-sixty merged commit 4be6653 into pydata:master Nov 20, 2020
@max-sixty
Copy link
Collaborator

Thanks @mathause !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic-internals topic-metadata Relating to the handling of metadata (i.e. attrs and encoding)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants