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

TYP: offsets.pyi #45331

Merged
merged 6 commits into from Feb 27, 2022
Merged

TYP: offsets.pyi #45331

merged 6 commits into from Feb 27, 2022

Conversation

@twoertwein
Copy link
Contributor

@twoertwein twoertwein commented Jan 12, 2022

This PR and #44922 are the last parts of #43744.

@pep8speaks
Copy link

@pep8speaks pep8speaks commented Jan 12, 2022

Hello @twoertwein! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-17 03:27:46 UTC

@@ -605,7 +605,8 @@ def asfreq(self, freq=None, how: str = "E") -> PeriodArray:

freq = Period._maybe_convert_freq(freq)

base1 = self.freq._period_dtype_code
# error: "BaseOffset" has no attribute "_period_dtype_code"
base1 = self.freq._period_dtype_code # type: ignore[attr-defined]
Copy link
Contributor Author

@twoertwein twoertwein Jan 12, 2022

Choose a reason for hiding this comment

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

This error occurs a few times. BaseOffset does not have this attribute but almost all(?) sub-classes of BaseOffset have this attribute.

Copy link
Member

@jbrockmendel jbrockmendel Jan 12, 2022

Choose a reason for hiding this comment

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

all(?)

Not all, no.

# treat this specifically as timedelta-NaT
result = np.empty(self.shape, dtype=np.float64)
result.fill(np.nan)
return result

# dispatch to Timedelta implementation
result = other.__rfloordiv__(self._ndarray)
return result
return other.__rfloordiv__(self._ndarray)
Copy link
Contributor Author

@twoertwein twoertwein Jan 12, 2022

Choose a reason for hiding this comment

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

Combined these two lines to not assign a new type to result

@twoertwein twoertwein marked this pull request as ready for review Jan 12, 2022
@overload
def __add__(self, other: npt.NDArray[np.object_]) -> npt.NDArray[np.object_]: ...
@overload
def __add__(self: _BaseOffsetT, other: BaseOffset) -> _BaseOffsetT: ...
Copy link
Contributor Author

@twoertwein twoertwein Jan 13, 2022

Choose a reason for hiding this comment

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

These operators are not "perfect" but they might be good enough for now. Based on the current annotations, __add__(self: Year, other: Second) is declared to return a Year but it actually returns Second

@jreback jreback added this to the 1.5 milestone Jan 22, 2022
@jreback
Copy link
Contributor

@jreback jreback commented Jan 22, 2022

@jbrockmendel
Copy link
Member

@jbrockmendel jbrockmendel commented Feb 15, 2022

@twoertwein can you merge main

@twoertwein
Copy link
Contributor Author

@twoertwein twoertwein commented Feb 15, 2022

@jbrockmendel rebased and green

@@ -219,7 +219,9 @@ def arithmetic_op(left: ArrayLike, right: Any, op):
# (https://github.com/pandas-dev/pandas/issues/41165)
_bool_arith_check(op, left, right)

res_values = _na_arithmetic_op(left, right, op)
# error: Argument 1 to "_na_arithmetic_op" has incompatible type
Copy link
Member

@jbrockmendel jbrockmendel Feb 16, 2022

Choose a reason for hiding this comment

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

likely avoidable if we inline should_extension_dispatch

freq._period_dtype_code == own_freq._period_dtype_code
freq._period_dtype_code
# error: "BaseOffset" has no attribute "_period_dtype_code"
== own_freq._period_dtype_code # type: ignore[attr-defined]
Copy link
Member

@jbrockmendel jbrockmendel Feb 16, 2022

Choose a reason for hiding this comment

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

i think this is a perf optimization for dtype == self.dtype that might no longer be necessary; i think we do this in PeriodDtype.eq now.

Copy link
Contributor Author

@twoertwein twoertwein Feb 17, 2022

Choose a reason for hiding this comment

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

Should probably be done in a separate PR.

pandas/_libs/tslibs/period.pyi Outdated Show resolved Hide resolved
@jreback
Copy link
Contributor

@jreback jreback commented Feb 27, 2022

@twoertwein status here?

@twoertwein
Copy link
Contributor Author

@twoertwein twoertwein commented Feb 27, 2022

@twoertwein status here?

I think it should be ready. The open two comments would require non-typing changes, which I would like to avoid in this PR.

@jreback jreback merged commit d391721 into pandas-dev:main Feb 27, 2022
32 checks passed
@jreback
Copy link
Contributor

@jreback jreback commented Feb 27, 2022

thanks @twoertwein

@jreback
Copy link
Contributor

@jreback jreback commented Feb 27, 2022

hmm maybe should have rebase first, oh well, let's see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants