Skip to content
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

Closed
wants to merge 5 commits into from

Conversation

xyuan
Copy link

@xyuan xyuan commented Aug 9, 2024

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

  • [y ] All fixes and/or new features come with corresponding tests.
  • Important design decisions have been documented in the approriate ADR inside the docs/development/ADRs/ folder.

If this PR contains code authored by new contributors please make sure:

  • All the authors are covered by a valid contributor assignment agreement provided to ETH Zurich and signed by the employer if needed.
  • The PR contains an updated version of the AUTHORS.md file adding the names of all the new contributors.

…g bugs requires untrim the unreachable nodes
)
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!")
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Author

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

Copy link
Contributor

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.

Copy link
Author

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

@FlorianDeconinck
Copy link
Contributor

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

@FlorianDeconinck
Copy link
Contributor

Won't do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants