-
Notifications
You must be signed in to change notification settings - Fork 183
puter - added accumulate, sum, abs, and more. #595
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
base: main
Are you sure you want to change the base?
Conversation
Cool!
Thanks, that was an oversight.
I don't think I realized implicit zeros weren't working, that's probably worth a ticket.
Yes, these were purposefully not put it yet. I'm actually hoping lambdas can be avoided all together, I'm not necessarily against them, but my goal with the Power Puter was to be powerful, but not necessarily exhaustive. |
py/power_puter.py
Outdated
| return result | ||
|
|
||
|
|
||
| def accumulate(iterable, func=None, initial=None): |
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.
Since itertools is standard I think you can just import it above, and change below to Function(name="accumulate", call=itertools.accumulate, args=(1, 2)),.
There would be additional work to make passing a different operation in, but op add will work as the default just the same.
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.
Fair point, though it will need to stay wrapped as otherwise it will return a itertools.accumulate object.
I would like to stay as close to python as possible, but it appears the initial argument of accumulate needs to be named.
list(itertools.accumulate(l, None, initial=8))So it's simply not possible to stay completely python compatible and retain initial.
I almost feel that it would be better to have it with a single argument and avoid the issue entirely.
Unless puter supports function definitions? That would be quite nice, it would allow you to return a function from one puter to use as an input in another puter.
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.
You're right that we would want to wrap the call so it gives back the iterable type you passed. But you should be able to pass named kwargs through.
Basically, this:
# power_puter.py
import itertools
# ...
def accumulate(iteratable, *args, **kwargs):
"""Wraps the itertools.accumulate call to return the iterable type."""
result = itertools.accumulate(iteratable, *args, **kwargs)
if isinstance(iteratable, list):
return list(result)
if isinstance(iteratable, set):
return set(result)
return tuple(result)
# ... and then the definition is:
Function(name="accumulate", call=accumulate, args=(1, 3)),Then in the power puter node, you can write out:
a = [1,2,3,4,5]
print(accumulate(a))
print(accumulate(a, initial=10))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 went with this hybrid approach, which is replete with issues, but seems the best solution that doesn't involve simply returning the accumulate iterator directly.
I'd want to actually test it before you commit it though. Just in case. tested
def accumulate(iterable, *args, **kwargs):
"""Wrap itertools.accumulate and return the same type as the input iterable."""
acc = itertools.accumulate(iterable, *args, **kwargs)
# If the user manages to get an exception using strings, then they deserve to receive it
if isinstance(iterable, (str, bytes, list, set, tuple)):
return type(iterable)(acc)
# If they pass a dict or bytearray or smth, then it's "best effort", and we need to make
# a copy incase it gets eaten during a failed attempt at type conversion.
result = list(acc)
try:
return type(iterable)(result)
except Exception:
return tuple(result)…rror, UnicodeError)
…rror, UnicodeError)
|
@rgthree since we've stalled here, I would suggest either removing |
|
(that would be my other account, apologies for confusion) |
I was driven a bit nuts when it took 5 attempts to achieve something fairly basic in your lovely puter, so I thought I would just make the modifications I needed rather than complain a lot in your issues section.
I added
sum,abs,accumulateand few more for good luck:mean,median,prod. I also modifiedminandmaxto accept single iterable arguments.I left slicing as is (doesn't like implicit 0's, e.g.,
l[:3]), and chose not to implement*(to unpack lists) since that sounded like it might break subtly stuff (though I might still do it). I also wholly avoided lambda functions, because... difficult, obviously.I tested it as best I could, and have attached image and workflow.
power_puter_test.json
Apologies that it takes two other large node collections to implement a simple test (one of them my own), but when it comes to testing I don't trust anybody else's nodes. I have been bitten too many times by various "Show Anything" nodes that can't differentiate a tuple from a comfy-wrapped OUTPUT_IS_LIST.