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.

Sign up for GitHub

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

Jump to bottom

Closes #1363: ak.Series with only values and more robust typechecking #1441

Merged
merged 1 commit into from
May 31, 2022
Merged

Closes #1363: ak.Series with only values and more robust typechecking #1441

merged 1 commit into from
May 31, 2022

Conversation

jeichert60
Copy link
Contributor

This PR (closes #1363):

  • Deprecates the ar_tuple parameter inside of the __init__ method.
  • Adjusts the tests to use the other 2 parameters (index, data) when instantiating.

@jeichert60 jeichert60 force-pushed the 1363_series_typecheck branch from 2d14d7f to fe9e583 Compare May 24, 2022 18:52
joshmarshall1
joshmarshall1 suggested changes May 24, 2022
View reviewed changes
Copy link
Contributor

@joshmarshall1 joshmarshall1 left a comment

Choose a reason for hiding this comment

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

Overall looks good, one small thing in the tail method and then a recommendation

arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
@joshmarshall1 joshmarshall1 mentioned this pull request May 24, 2022
Merged
@jeichert60 jeichert60 force-pushed the 1363_series_typecheck branch 3 times, most recently from e003e66 to 95513a5 Compare May 24, 2022 19:30
stress-tess
stress-tess approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Couple minor suggestions, looks good overall!

arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
Ethan-DeBandi99
Ethan-DeBandi99 suggested changes May 25, 2022
View reviewed changes
Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

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

Josh and Pierce touched on most things already. After discussing with @pierce314159 yesterday, I believe we can actually remove the ar_tuple parameter, but allow for data to handle a Tuple and List as well. This effectively will not change the api because it looks like most places ar_tuple is being used, it is being done positionally. We could then add an error using **kwargs to detect if ar_tuple is passed as a keyword. I think this eliminates some technical debt moving forward and removes the need for the deprecation warning. Alternatively, if we want to leave the deprecation warning, but still allow ar_tuple to be usable for a release cycle, I think that would be fine as well. This would also remove the need to update a lot of the usage for the Series.__init__ to utilize the new parameter format.

arkouda/series.py Outdated Show resolved Hide resolved
@jeichert60 jeichert60 force-pushed the 1363_series_typecheck branch 3 times, most recently from 2159b58 to 31d7c19 Compare May 26, 2022 18:46
stress-tess
stress-tess reviewed May 26, 2022
View reviewed changes
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

few small things

arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
arkouda/series.py Outdated Show resolved Hide resolved
tests/series_test.py Outdated Show resolved Hide resolved
@jeichert60
Closes Bears-R-Us#1363: ak.Series with only values and more robust ty…
13abc83 
…pechecking

This PR (closes Bears-R-Us#1363):

- Deprecates the `ar_tuple` parameter inside of the `__init__` method.
- Adjusts the tests to use the other 2 parameters `(index, data)` when instantiating.
@jeichert60 jeichert60 force-pushed the 1363_series_typecheck branch from 31d7c19 to 13abc83 Compare May 27, 2022 13:05
stress-tess
stress-tess reviewed May 31, 2022
View reviewed changes
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

few things, mostly documentation

arkouda/series.py Show resolved Hide resolved
arkouda/series.py Show resolved Hide resolved
tests/series_test.py Show resolved Hide resolved
@stress-tess stress-tess merged commit c0786e6 into Bears-R-Us:master May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stress-tess stress-tess stress-tess approved these changes

@joshmarshall1 joshmarshall1 joshmarshall1 approved these changes

@Ethan-DeBandi99 Ethan-DeBandi99 Ethan-DeBandi99 approved these changes

@mhmerrill mhmerrill Awaiting requested review from mhmerrill

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ak.Series with only values and more robust typechecking
4 participants
@jeichert60 @Ethan-DeBandi99 @stress-tess @joshmarshall1