search

inducer / pymbolic

A simple package to do symbolic math (focus on code gen and DSLs)
http://mathema.tician.de/software/pymbolic
Other
106 stars 24 forks source link

Cached mappers can't handle lists #100

Closed zachjweiner closed 2 years ago

zachjweiner commented 2 years ago

The new caching mappers seem to balk at lists (or any unhashable type), since only KeyErrors are caught here.

Actually, there appears to be have already been a "CachingMapperMixin" that handles the corresponding TypeErrors appropriately. Is there a reason for the distinction here or could it be combined with CachedMapper?

For context, I subclass loopy's newly caching-enabled mappers and for convenience enable them to recurse over lists and dicts (usually as the outermost structures).

inducer commented 2 years ago

I'm guessing the data structures you're using are de facto immutable. The cleanest thing IMO would be to switch to actually immutable data structures if that's possible. (tuple, immutables.Map). Is that in the cards for your use case?

inducer commented 2 years ago

Relatedly, I'm guessing this is for pystella. Would you want to include that in our downstream CI to allow us to not accidentally break your code?

zachjweiner commented 2 years ago

I'm guessing the data structures you're using are de facto immutable. The cleanest thing IMO would be to switch to actually immutable data structures if that's possible. (tuple, immutables.Map). Is that in the cards for your use case?

I think that's the case (and the morally right thing to do), but I'd probably sooner be lazy just override __call__ to behave like CachingMapperMixin and simply recurse on any unhashable object. For me that's totally fine in effect, since lists and dicts are usually just expression containers that are mapped only once. (Edit: the containers are mapped once, not necessarily the contained expressions.)

But I might still argue that the base cached mappers (which do inherit map_list methods, I think) could more gracefully handle lists (or numpy arrays, or whatever data structures the noncaching mappers support), whether by opting out of caching in the same way as that other CachingMapperMixin or by yelling about it. (The current exception, e.g. line 69 here, is obviously not useless, but does require navigating to the pymbolic source to know what's going on.)

Would you want to include that in our downstream CI to allow us to not accidentally break your code?

Thanks very much for offering! But actually, no - I definitely wouldn't want (or need) to ever hold you up for that reason and am happy to let weekly CI keep me apprised. My own work is pinned to my streaming prefetch branch anyway, which I rebase on main on the semi-annual occasion that I feel like doing so. No need for the extra carbon =)

zachjweiner commented 2 years ago

Oh, PS: loopy's pymbolic dep needs a version bump.

inducer commented 2 years ago

Thanks! https://github.com/inducer/loopy/pull/619

zachjweiner commented 2 years ago

@inducer - just so this issue doesn't needlessly linger, let me know if you're amenable to a patch along the lines of my suggestion, otherwise feel free to close this.

inducer commented 2 years ago

I kind of feel like extending CachedMapper to deal with mutable structures is inviting mistakes. I also view map_list as a mistake, so I'm not super-inclined to perpetuate it. So maybe that's a patch that I'd ask you to carry in your branch.

Btw thanks for reporting the CachingMapperMixin /CachedMapper duplication. #103 deprecates CachingMapperMixin.

zachjweiner commented 2 years ago

Totally reasonable, thanks for your thoughts!