Skip to content

Owo #1

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Owo #1

wants to merge 7 commits into from

Conversation

fpunny
Copy link

@fpunny fpunny commented Mar 31, 2020

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

relabeling = func is None and is_multi_agg_with_relabel(**kwargs)
if relabeling:
# OWO CHANGES
Copy link
Author

Choose a reason for hiding this comment

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

We should make this into a new normalize_keyword_aggregation

Copy link
Author

Choose a reason for hiding this comment

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

While we're at it, check if second field of tuple is a lambda, since native aggregation methods wouldn't work obviously

@@ -393,7 +406,16 @@ def _agg(arg, func):
"""
result = {}
for fname, agg_how in arg.items():
result[fname] = func(fname, agg_how)
# OWO CHANGES
Copy link
Author

Choose a reason for hiding this comment

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

Perhaps encapsulate the changes in a new agg function to keep some consistency (agg_multi_d1? or something)

relabeling = func is None and is_multi_agg_with_relabel(**kwargs)
if relabeling:
# OWO CHANGES
import json
Copy link
Author

@fpunny fpunny Apr 3, 2020

Choose a reason for hiding this comment

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

We can explore other tools like np.array().toBytes() (if more efficient) and also use the python list_like check thing

Choose a reason for hiding this comment

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

Depends on if JSON is too slow and if they can't cythonize this code

Copy link

Choose a reason for hiding this comment

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

Implemented

items = json.loads(fname)
_obj = {}
for item in items:
_obj[item] = self._gotitem(item, ndim=1, subset=None)
Copy link
Author

@fpunny fpunny Apr 3, 2020

Choose a reason for hiding this comment

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

This is kinda bad... but hopefully with a formal PR, we can get feedback on how to do this better (maybe pipeline method would work better if we are to keep internal consistency)

@@ -393,7 +406,16 @@ def _agg(arg, func):
"""
result = {}
for fname, agg_how in arg.items():
result[fname] = func(fname, agg_how)
# OWO CHANGES
import json

Choose a reason for hiding this comment

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

might have some issues with json performance that we need to discuss

@@ -0,0 +1,81 @@
{

Choose a reason for hiding this comment

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

might want to delete this or concert it into a testcase

@wgranados
Copy link

Missing documentation for this feature, so I'll add this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants