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

Added implementation of kurt #23

Merged
merged 1 commit into from
Jan 22, 2024
Merged

Added implementation of kurt #23

merged 1 commit into from
Jan 22, 2024

Conversation

MiguelGomezC
Copy link

@MiguelGomezC MiguelGomezC commented Jan 4, 2024 •
edited
Loading

Provides an implementation for the kurt method.

Solves: /issues/14

Parameters:

Name Type Description Default
axis int Axis for the function to be applied on. 0 is columns, 1 is rows. 0
skipna bool not yet implemented True
numeric_only bool Only use columns of the table that are of a numeric data type. False

Returns:

Type Description
Dictionary Map of columns and their yielded kurtosis values

It follows the pandas definition.

@nipsn nipsn linked an issue Jan 4, 2024 that may be closed by this pull request
Feature: Implement kurt from Pandas API #14
Closed
tortolavivo23
tortolavivo23 suggested changes Jan 4, 2024
View reviewed changes
src/pykx/pandas_api/pandas_meta.py Outdated Show resolved Hide resolved
@MiguelGomezC MiguelGomezC force-pushed the feature/pandas-api-kurt branch 3 times, most recently from f8095fe to 33c8735 Compare January 5, 2024 14:28
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 5, 2024
@MiguelGomezC MiguelGomezC self-assigned this Jan 5, 2024
nipsn
nipsn suggested changes Jan 5, 2024
View reviewed changes
src/pykx/pandas_api/pandas_meta.py Outdated Show resolved Hide resolved
docs/user-guide/advanced/Pandas_API.ipynb Show resolved Hide resolved
docs/user-guide/advanced/Pandas_API.ipynb Outdated Show resolved Hide resolved
docs/user-guide/advanced/Pandas_API.ipynb Outdated Show resolved Hide resolved
@MiguelGomezC MiguelGomezC force-pushed the feature/pandas-api-kurt branch from bff087f to 77e8617 Compare January 12, 2024 14:10
@MiguelGomezC MiguelGomezC changed the base branch from main to feature/pandas-api-2nd-block January 12, 2024 14:13
@MiguelGomezC MiguelGomezC requested a review from nipsn January 15, 2024 08:21
@MiguelGomezC MiguelGomezC force-pushed the feature/pandas-api-kurt branch from 77e8617 to 1098143 Compare January 15, 2024 09:28
@MiguelGomezC MiguelGomezC marked this pull request as ready for review January 15, 2024 09:32
@MiguelGomezC MiguelGomezC force-pushed the feature/pandas-api-kurt branch 2 times, most recently from 99bdb1b to 1275111 Compare January 15, 2024 09:35
chraberturas
chraberturas approved these changes Jan 16, 2024
View reviewed changes
Copy link

@chraberturas chraberturas left a comment

Choose a reason for hiding this comment

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

Overall, it's a good job; I only have those minor questions 👍

src/pykx/pandas_api/pandas_meta.py
return q(
'{[tab]'
f'r:{{[tab; x] ({key_str}x; {kurt_str} {val_str}tab[x])}}[tab;] each {query_str};'
f'(,/) {{(enlist x 0)!(enlist x 1)}} each r{where_str}}}',
Copy link

Choose a reason for hiding this comment

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

Why do you use (,/) instead of using raze? Same length and I think it's easier to understand

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps I would keep it consistent with the functions the KX team already implemented. Even if some amount of boilerplate is generated, they seem to stick to using this design pattern (for instance, in the implementation of median, or mean). Then after the fact, should they make a refactorization or an improvement on the design pattern, to be implemented on all of the existing functions that use it. What do you think?

src/pykx/pandas_api/pandas_meta.py
return q(
'{[tab]'
f'r:{{[tab; x] ({key_str}x; {kurt_str} {val_str}tab[x])}}[tab;] each {query_str};'
f'(,/) {{(enlist x 0)!(enlist x 1)}} each r{where_str}}}',
Copy link

Choose a reason for hiding this comment

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

Again, I think enlist[x 0]!enlist x 1 is shorter and easier to understand

Copy link
Author

Choose a reason for hiding this comment

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

Same argument to be made as for the raze case. Maybe I would keep it as is for now until they make an adjustment to the design pattern.

@MiguelGomezC MiguelGomezC force-pushed the feature/pandas-api-kurt branch 2 times, most recently from e303314 to 44b7063 Compare January 22, 2024 14:57
@MiguelGomezC MiguelGomezC force-pushed the feature/pandas-api-kurt branch from 44b7063 to f856311 Compare January 22, 2024 14:59
@MiguelGomezC MiguelGomezC merged commit ea1a59e into feature/pandas-api-2nd-block Jan 22, 2024
1 check passed
@MiguelGomezC MiguelGomezC mentioned this pull request Jan 22, 2024
Closed
nipsn pushed a commit that referenced this pull request Feb 8, 2024
@nipsn
Added implementation of kurt function (#23)
a56f6b5
nipsn added a commit that referenced this pull request Feb 13, 2024
@nipsn @tortolavivo23
Add kurt and sem functions (#32)
bb1549a 
* Added implementation of kurt function (#23)

* Added implementation of sem function (#22)

* Refactored kurt and sem functions

---------

Co-authored-by: Miguel Gómez <[email protected]>
Co-authored-by: Francisco Tórtola Vivo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcosvm13 marcosvm13 marcosvm13 approved these changes

@tortolavivo23 tortolavivo23 tortolavivo23 approved these changes

@nipsn nipsn nipsn approved these changes

@cperezln cperezln Awaiting requested review from cperezln

@chraberturas chraberturas Awaiting requested review from chraberturas

Assignees

@MiguelGomezC MiguelGomezC

Labels
documentation Improvements or additions to documentation python tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: Implement kurt from Pandas API
5 participants
@MiguelGomezC @nipsn @tortolavivo23 @marcosvm13 @chraberturas