Cached mappers can't handle lists about pymbolic HOT 8 CLOSED

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?

from pymbolic.

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?

from pymbolic.

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 =)

from pymbolic.

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

from pymbolic.

Thanks! inducer/loopy#619

from pymbolic.

@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.

from pymbolic.

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.

from pymbolic.

Totally reasonable, thanks for your thoughts!

from pymbolic.