-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix deep_map
#138
base: main
Are you sure you want to change the base?
Fix deep_map
#138
Conversation
…_call__()` to fill `input_tables`)
I'm not sure if this is the correct approach to solve this issue. Either convert
I don't know what the optimal solution for this problem would be. Some thing I would like to note:
|
How about converting Iterables != tuple to list and Mapping to dict? if cls == tuple:
y = _deep_map_tuple(x, fn, memo)
elif isinstance(cls, Mapping):
y = _deep_map_dict(x, fn, memo)
elif isinstance(cls, Iterable):
y = _deep_map_list(x, fn, memo)
else:
y = fn(x) |
@NMAC427 what do you think about my proposal to make any Mapping a dictionary and any Iterable != tuple a list? |
@windiana42 it depends on how exactly you identify mappings and iterables. For example, |
Theoretically any user code could quite easily extend deep_map for any arbitrary iterable by providing the appropriate mapping function. For example: def f(x):
if isinstance(x, SomeContainer):
return deep_map(f, list(x))
return str(x)
c = SomeContainer(...)
deep_map(f, c) However, the inverse (preventing deep_map from deep mapping an iterable) is impossible. Therefore we should be quite careful to ensure we don't accidentally make the problem worse. |
I think dict().values() should be converted to list or tuple. I think list, tuple, and dict deserve extra status in the sense that we want to keep them in-tact. We should make sure nothing bad happens to pipedag.Table() objects and sources of auto-table conversion should also be kept as is until they are converted to pipedag.Table(). |
Currently
deep_map
is not aware ofdict_values
. This resulted ininput_tables
not being populated inMaterializationWrapper.__call__()
.Checklist
docs/source/changelog.md
entry