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

Added implementation of values #28

Merged
merged 13 commits into from
Jan 19, 2024

Conversation

cperezln
Copy link

@cperezln cperezln commented Jan 11, 2024

Feature

  • Please insert link to associated issue here: /issues/15

What does this change introduce?

An implementation of the values function: https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.values.html

This is intended to be a 1:1 implementation of the values function from pandas.

General

  • Has an example been added to demo the new feature?
  • Have existing examples been updated or tested?
  • Have you added any new Environment Variables/Configuration Options? If yes please tick the boxes below as applicable
    • Addition to reimporter logic within src/pykx/pykx.q and src/pykx/reimporter.py
    • Have updated the src/pykx/util.py logic which is used for environment variable
  • If there have been any dependency updates have they been reflected in all files?
    • pyproject.toml
    • docs/getting-started/installing.md
    • conda-recipe/meta.yaml
    • README.md
  • If any examples have been updated has it's associated .zip been updated

Code

  • Has all temporary code used during development been removed?
  • Has all commented out (unused) code been removed?
  • Where reasonable have you ensured there is no duplication of existing code?
  • If applicable for your use-case have you ensured that the code is performant?

Testing

  • Have unit tests been created or existing ones updated to test this new functionality?

Documentation

  • Has documentation been added for all public code?
  • Has a release note been included for the new feature?
  • Has any documentation which would benefit from this feature been updated to use the most up to date functionality?
  • If a new class has been added has a documentation stub .md file associated with it been created?
  • If any documentation page has been created has it been added to mkdocs.yml
  • Have you checked your changes with a spell checker? (US English)

@cperezln cperezln requested a review from marcosvm13 January 11, 2024 13:08
@cperezln cperezln self-assigned this Jan 11, 2024
@github-actions github-actions bot added documentation Improvements or additions to documentation python tests labels Jan 11, 2024
@marcosvm13 marcosvm13 added this to the Pandas API 2nd Block milestone Jan 12, 2024
@marcosvm13 marcosvm13 added the work in progress Working on it label Jan 12, 2024
@marcosvm13 marcosvm13 linked an issue Jan 12, 2024 that may be closed by this pull request
@cperezln cperezln changed the title Feature/pandas api values Added implementation of values Jan 16, 2024
@cperezln cperezln changed the base branch from main to feature/pandas-api-2nd-block January 16, 2024 14:19
@cperezln cperezln removed the work in progress Working on it label Jan 16, 2024
@cperezln cperezln marked this pull request as ready for review January 16, 2024 14:23
Copy link

@nipsn nipsn left a comment

Choose a reason for hiding this comment

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

Overall the implementation is on the right track, but there are a couple of aspects that need to be checked. Mainly:

  • Does the q session run with multithreading enabled on PyKX? I will ask the devs and get back to you so you can decide what to do next.
  • The tests need a bit of work. I think a specific case with null values would be helpful to see if they are handled properly.
  • The assertion itself it's a bit tricky, so feel free to iterate over my suggestions.

tests/test_pandas_api.py Outdated Show resolved Hide resolved
tests/test_pandas_api.py Outdated Show resolved Hide resolved
tests/test_pandas_api.py Outdated Show resolved Hide resolved
tests/test_pandas_api.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_meta.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_meta.py Outdated Show resolved Hide resolved
docs/user-guide/advanced/Pandas_API.ipynb Outdated Show resolved Hide resolved
tests/test_pandas_api.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_meta.py Outdated Show resolved Hide resolved
@cperezln cperezln requested a review from nipsn January 18, 2024 08:46
Copy link

@nipsn nipsn left a comment

Choose a reason for hiding this comment

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

Looks good to me now 👍

},
{
"cell_type": "code",
"execution_count": 2,

Choose a reason for hiding this comment

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

The value of execution_count should be null, like in the others code cells

@cperezln cperezln merged commit eaea633 into feature/pandas-api-2nd-block Jan 19, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation python Ready to review tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Implement values from Pandas API
5 participants