-
Notifications
You must be signed in to change notification settings - Fork 49
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
release the assert to be warning message to allow relaxed checking #1612
Conversation
…g bugs requires untrim the unreachable nodes
src/gt4py/cartesian/gtc/daceir.py
Outdated
) | ||
if (first_start <= second_end and first_end >= second_start) or ( | ||
second_start <= first_end and second_end >= first_start) : | ||
warnings.warn(f"extension is NOT monotonically increasing!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give context. The assert makes sure that the intersection is not empty. I agree that it could make sense to represent an empty DomainInterval, but then I would not give a warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind whether we remove this warning message or not, I give it as a warning is to use it as a reminder for the users or app developers the stencil is void, this is needed for pyFV3 corner update stencils as it is void stencils when there is no corner info to update. sorry for delayed response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@havogt yeah, I remove the warning message, just release the assert statement and allow the void stencil, please review it again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need more context to review this. Can you add a testcase that exercises this new code path? Alternatively, if @FlorianDeconinck wants to take the responsibility and just approve, it's ok for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, will add a testcase
This will indeed (in coordination with node pruning removal) take care of our empty stencil predicament. But a better fix would go through a refactor of the gt4py<>bridge code to have more code exposed directly to the SDFG. This refactor is ongoing. We will check status there and come up with a plan |
Won't do |
This changes allow the unreachable code to be released with non-monotonic increasing field extensions of loop, as it trimmed the function scalar arguments, and cause crash in our code. This is bug fix.
PR Title: release the assert to be warning message to allow relaxed checking
:
- build: Changes that affect the build system or external dependencies
- ci: Changes to our CI configuration files and scripts
- docs: Documentation only changes
- feat: A new feature
- fix: A bug fix
- perf: A code change that improves performance
- refactor: A code change that neither fixes a bug nor adds a feature
- style: Changes that do not affect the meaning of the code
- test: Adding missing tests or correcting existing tests
: cartesian | eve | next | storage
ONLY if changes are limited to a specific subsytem
PR Description:
Description of the main changes with links to appropriate issues/documents/references/...
-->
Description
Requirements
If this PR contains code authored by new contributors please make sure:
AUTHORS.md
file adding the names of all the new contributors.