inducer / arraycontext

Choose your favorite numpy-workalike!
6 stars 11 forks source link

Make `get_container_context_recursively` return actx or error #149

Closed inducer closed 2 years ago

inducer commented 2 years ago

This is a deliberate compatibility break based on seeing code like this:

https://github.com/inducer/grudge/blob/62af257a45be631e2bf1d3c4cf9de4ec746f375d/grudge/trace_pair.py#L307

which clearly would not succeed if the return value was None, based on the notion that the original get_container_context_recursively is easy to misuse because of its Optional return value. The rest of this is a renaming of get_container_context to get_container_context_opt for consistency.

As for the compatibility break, my justification is that all existing user code seems to be using the new get_container_context_recursively already. :)

alexfikl commented 2 years ago

If I understood correctly, the idea is that if someone calls get_array_context_recursively they must really want a context, so it should yell more loudly if it doesn't find one?

inducer commented 2 years ago

Yep, that's the idea.

inducer commented 2 years ago

Given the mirgecom failure, I think I might start by warning about None returns for now, patch up mirgecom, and then put the rest of this plan into action.

alexfikl commented 2 years ago

Given the mirgecom failure, I think I might start by warning about None returns for now, patch up mirgecom, and then put the rest of this plan into action.

Maybe add a strict flag like a few of the other functions and warn when people set it to a deprecated strict=False? Nevermind!