upgrade 'unevaluated array as argument' warning to error - #305
Conversation
logger.warning does not deduplicate, in contrast to warnings.warn
Could you explain the context here? I'd imagine that this will be awfully noisy, for (IMO) not much gain. Ultimately, one should fix these warnings until they're all gone. |
My thought here was that in contrast to most (almost all?) other warnings, this type of warning can have serious performance implications. I was surprised to see this warning appear in the shock1d driver, and they appeared at a point during the execution where many other warnings are displayed, and thus this kind of warning tends to "disappear". I'll set this back to draft, I think we should probably also show the name of the compiled function. |
alexfikl
commented
Mar 19, 2025
You can set
If that's the case, wouldn't it be better to disable all warnings on a "production run"? |
matthiasdiener
commented
Mar 19, 2025
TIL, thanks. The interface is a bit clumsy, but it seems to work.
Sorry, my text probably wasn't very clear here. What I meant is that the issue pointed out by the warnings could have serious implications. |
Oops, sorry, I wasn't clear there.. I meant that you can just filter warnings in your top-level driver, not here. I agree that it looks very clumsy as is :(
Ah, so the issue is that the warning is not loud enough? I would still argue that this is better done on the application side with |
| "considerable expense. This is deprecated and will stop " | ||
| "working in 2023. To avoid this warning, force evaluation " | ||
| "of all arguments via freeze/thaw.", | ||
| stacklevel=4) |
There was a problem hiding this comment.
I think it'd be useful to create a special warning class for this:
class UnevaluatedCompiledFuncArgWarning(UserWarning):
pass
(and then pass the appropriate argument to warn) This makes precise filtering from the application side possible. I don't know that I want to install a warning filter on the user's behalf. (We could, in the actx constructor, on an opt-out basis? But that still feels overbearing.)
Alternatively, we could just make this an error, given that it has been deprecated with a deadline that has passed.
There was a problem hiding this comment.
I'm starting to like the "make it an error" idea, I think.
There was a problem hiding this comment.
I'm starting to like the "make it an error" idea, I think.
Same here :)
inducer
commented
Mar 20, 2025
inducer/grudge#376 should address the grudge failures. |
inducer
left a comment
There was a problem hiding this comment.
LGTM, just one question, hopefully quick.
| from .utils import get_cl_axes_from_pt_axes | ||
| from arraycontext.impl.pyopencl.taggable_cl_array import to_tagged_cl_array | ||
| fn_name = self.pytato_program.program.entrypoint |
There was a problem hiding this comment.
This and the fn_names below appear to be inconsistent. Is there a specific reason for this?
There was a problem hiding this comment.
Thanks, this is hopefully fixed in b1c3a73. Part of the challenge here is that CompiledPyOpenCLFunctionReturningArray appears to be unused.
inducer
commented
Mar 26, 2025
Thx! |
majosm
commented
Mar 27, 2025
FYI @inducer@matthiasdiener, this change appears to break the non-compiled time integrators in grudge/mirgecom. (Looks like grudge downstream CI here doesn't run the examples, so it missed that.) |
inducer
commented
Mar 27, 2025
As well it should! I'll get grudge fixed up. |
inducer
commented
Mar 27, 2025
See inducer/grudge#377 and #307. |
Please squash