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

Inner contribution review: count, round, std, etc. #11

Closed
wants to merge 6 commits into from

Conversation

neutropolis
Copy link
Member

Feature

  • Please insert link to associated issue here:

What does this change introduce?

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)

@github-actions github-actions bot added documentation Improvements or additions to documentation python tests labels Dec 14, 2023
Copy link
Member Author

@neutropolis neutropolis left a comment

Choose a reason for hiding this comment

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

I've just added the very same comments we had at the meeting. I still have to review round and the prefix & suffix functions.

Another question, does the order of the new functions make sense? They seem to be grouped and ordered alphabetically.

\cc @nipsn @marcosvm13 @MiguelGomezC

src/pykx/pandas_api/pandas_meta.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_meta.py Show resolved Hide resolved
)
return res


Copy link
Member Author

Choose a reason for hiding this comment

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

This line should be removed.

src/pykx/pandas_api/pandas_meta.py Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
@marcosvm13 marcosvm13 force-pushed the feature/round_prefix_suffix_std_skew-REVIEW branch from 1944f5f to fd68c73 Compare December 15, 2023 14:22
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
src/pykx/pandas_api/pandas_meta.py Show resolved Hide resolved
src/pykx/pandas_api/pandas_meta.py Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Show resolved Hide resolved
src/pykx/pandas_api/pandas_indexing.py Outdated Show resolved Hide resolved
@@ -210,6 +234,27 @@ def abs(self, numeric_only=False):
tab = _get_numeric_only_subtable(self)
return q.abs(tab)

@api_return
def round(self, decimals: Union[int, Dict[str, int]] = 0):
Copy link

Choose a reason for hiding this comment

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

@neutropolis this is my current reimplementation of round. It looks way more "pythonic", but in the end I still needed to write a couple of auxilary functions in the same fashion of ones that already exist within the library.

def _type_num_is_f_or_e(typenum):
    return typenum == 8 or typenum == 9

def _get_real_or_float_cols(tab):
    t = q('0#0!', tab)
    cols = q.cols(t).py()
    return [col for col in cols if _type_num_is_f_or_e(t[col].t)]

...

@api_return
def round(self, decimals: Union[int, Dict[str, int]] = 0):
    tab = self
    if 'Keyed' in str(type(tab)):
        tab = q('{(keys x) _ 0!x}', tab)

    numeric_cols = _get_real_or_float_cols(tab)

    generate_ops_f = q("""{key[x]!({(({"F"$.Q.f[y]x}[;x[1]])';x[0])}'){flip(2;count[x])#key[x],value[x]}[x]}""")

    if type(decimals) is int:
        validated = dict([(col, decimals) for col in numeric_cols])
    else:
        validated = dict([(k, v) for k, v in decimals.items() if k in numeric_cols])

    return q("{![x;();0b;y]}", tab, generate_ops_f(validated))

The auxilary functions resemble _type_num_is_numeric_or_bool and _get_numeric_only_subtable, found within the same file. Maybe I should rename the first one to type_num_is_float_or_real or something among those lines?

As for round, I could not get rid of the if else because of the typing of decimals; either I generate one string or the other, but can't really do both. Getting rid of the floor case made the code way simpler, but I will need to add and change some unit tests. How does it look so far?

I still have not pushed my changes because I need to sort out the typing issues (keep the reals and floats in their original types) and add those unit tests.

@marcosvm13 marcosvm13 force-pushed the feature/round_prefix_suffix_std_skew-REVIEW branch from c776c9a to 0d2a6a8 Compare December 18, 2023 10:21
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 tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants