-
Notifications
You must be signed in to change notification settings - Fork 97
Adds overloads to ak.full #5223
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
base: main
Are you sure you want to change the base?
Conversation
ff4a621 to
6f1020e
Compare
6a7ec72 to
43efbc2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5223 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 4
Lines ? 63
Branches ? 0
========================================
Hits ? 63
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
43efbc2 to
5c5a71b
Compare
ajpotts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
57672b3 to
cc107c5
Compare
430ae7a to
a050f8e
Compare
1RyanK
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
| fill_value: Union[numeric_and_bool_scalars, str_scalars], | ||
| dtype: str, | ||
| max_bits: Optional[int] = ..., | ||
| ) -> Strings: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think although this doesn't "cause problems" with mypy, it does make mypy believe the wrong thing. For example, if you do ak.full(5, 7, dtype="int64"), mypy believes a Strings object will be returned. Here's a wacky, roundabout way to get that to show up:
In [13]: import textwrap, tempfile, os, sys, subprocess, pathlib
...:
...: code = textwrap.dedent("""
...: import arkouda as ak
...: import numpy as np
...:
...: def f() -> None:
...: a = ak.full(5, 7, dtype="int64")
...: reveal_type(a)
...:
...: b = ak.full(5, 7, dtype="str")
...: reveal_type(b)
...:
...: c = ak.full(5, "x")
...: reveal_type(c)
...:
...: d = ak.full(5, np.nan)
...: reveal_type(d)
...: """)
...:
...: with tempfile.TemporaryDirectory() as td:
...: p = pathlib.Path(td) / "mypy_check.py"
...: p.write_text(code)
...:
...: # Run mypy. You might need: `pip install mypy` in your env first.
...: res = subprocess.run(
...: ["mypy", str(p)],
...: capture_output=True,
...: text=True,
...: )
...: print(res.stdout)
...: print(res.stderr)
...:
/tmp/tmpy9644sti/mypy_check.py:7: note: Revealed type is "arkouda.numpy.strings.Strings"
/tmp/tmpy9644sti/mypy_check.py:10: note: Revealed type is "arkouda.numpy.strings.Strings"
/tmp/tmpy9644sti/mypy_check.py:13: note: Revealed type is "arkouda.numpy.strings.Strings"
/tmp/tmpy9644sti/mypy_check.py:16: note: Revealed type is "arkouda.numpy.pdarrayclass.pdarray"
Success: no issues found in 1 source fileNotably, when it does
a = ak.full(5, 7, dtype="int64")
reveal_type(a)The corresponding output is
/tmp/tmpy9644sti/mypy_check.py:7: note: Revealed type is "arkouda.numpy.strings.Strings"But if you actually try it, you get int64. Maybe try
...
@overload
def full(
size: ...,
fill_value: Union[numeric_and_bool_scalars, str_scalars],
dtype: Literal["str", "string"],
max_bits: Optional[int] = ...,
) -> Strings: ...
Closes #5173
This one required a fair amount of work to satisfy mypy once the overloads were written. I'm going to keep it in draft initially.
Notes:
In addition to the work on the overloads, mypy found an error in the order of calling parameters to full in my recent PR that changed ak.minimum and ak.maximum. So that's been fixed.
Once the overloads were in place, mypy flagged an error in the use of full in isna function in the recent pandas extensions. I addressed that by converting the result of ak.full to an ArkoudaArray before returning.