-
Notifications
You must be signed in to change notification settings - Fork 15
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
Addition of skew, add_prefix, add_suffix, round, count, std functions #15
Addition of skew, add_prefix, add_suffix, round, count, std functions #15
Conversation
@cmccarthy1 @rianoc-kx |
Thanks @nipsn we'll aim to review before the end of the week and get back to you with any feedback/updates! Thanks for the contribution |
Hi @cmccarthy1 I just came back after my leave, please let me a few days to take a look at these changes before handing them back to you, just to check our progress. Thanks! |
elif axis == 1: | ||
t = _pre_suf_fix_index(t, suffix, suf=True) |
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 don't think we should support the conversions of this type for indexes unless the columns in the index are symbol/string types. Adding a suffix to integer/guid/float etc would have a number of implications on the symbol count within the processes which would have a memory impact. Additionally it will update the type of the columns which is possibly fine but probably worth requiring a user to convert to the symbol types before doing the index in that case
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.
We've decided to remove this logic and just fail with a nyi
error for this particular situation.
@@ -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): |
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.
This function needs a couple of changes before it could be merged, at present it only handles float
values but should probably also handle real
values also (type -8h).
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.
After further reviewing, we could see that round
is tricky, and we've decided to remove it from this contribution.
generate_ops:{[vdic] | ||
tuples:{flip(2;count[x])#key[x],value[x]}[vdic]; | ||
key[vdic]!({(({"F"$.Q.f[y]x}[;x[1]])';x[0])}')tuples}; | ||
get_float_cols:{(key[ct]@where 9=value[ct:abs type each first x])}; |
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.
Using first
to determine the type of a column isn't foolproof as you may be dealing with a column that is a mixed list, for example
([]10?1f;10?1f;1f,9?`a`b`c)
This would determine column x2
to be a float column rather than a mixed list of type 0h
A more general way of doing this would be the following and it allows for extension to other types (reals in this case)
get_cols:{metaTab:0!meta x;metaTab[`c]where metaTab[`t]in y}
This can then be called passing in "fe"
along with the table to retrieve the float and real columns of the table
get_cols[table;"fe"]
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.
Please, refer to previous comment.
No issues @neutropolis I've added a couple of small comments to ponder but will hold off digging deeper until after you've reviewed further |
Hi @cmccarthy1 sorry for the delay, it took us longer than expected to review the code. In essence, we've decided to remove |
Feature
What does this change introduce?
An implementation of the
skew
function:https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.skew.html#pandas.DataFrame.skew
An implementation of the
add_prefix
andadd_suffix
function:https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.add_prefix.html
https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.add_suffix.html
Observations:
An implementation of the
round
function:https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.round.html#pandas.DataFrame.round
Observations:
An implementation of the
count
function:Observations:
An implementation of the
std
function:https://pandas.pydata.org/docs/reference/api/pandas.DataFrame.std.html#pandas.DataFrame.std
General
src/pykx/pykx.q
andsrc/pykx/reimporter.py
src/pykx/util.py
logic which is used for environment variable.zip
been updatedCode
Testing
Documentation
.md
file associated with it been created?mkdocs.yml