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

fix[cartesian]: Race condition in unit test for K write #1791

Conversation

FlorianDeconinck
Copy link
Contributor

The interval analysis in the unit test test_K_offset_write_conditional fails to catch a mistake in the code that leads to a race condition.

Work:

  • Fix the bad interval
  • Remove not needed restriction on CUDA version

Further work to fix the underlying problem and the larger issue of bound check on variable indexing is covered here and there

Remove not needed restriction on CUDA version
@havogt
Copy link
Contributor

havogt commented Jan 9, 2025

I am not any more familiar enough with cartesian. What's the expected behavior? Does the user need to protect the oob in the vertical interval or should gt4py take care but it's buggy?

@FlorianDeconinck
Copy link
Contributor Author

FlorianDeconinck commented Jan 9, 2025

I am not any more familiar enough with cartesian. What's the expected behavior? Does the user need to protect the oob in the vertical interval or should gt4py take care but it's buggy?

Sorry I put this PR in a rush I should have explained a bit better.

This PR is aimed at stabilizing the CI first and foremost. The issue is dual:

Now of course this is part of the larger discussion on physics and there's a definite need for us to sit down and probably talk it through as I am very well aware that we are stepping beyond the bounds of the original design of cartesian.

But we are really responding to the need of the algorithm and our current wider strategy is to not rewrite the algorithm

…st_code_generation.py

Co-authored-by: Hannes Vogt <hannes@havogt.de>
@havogt havogt merged commit 1601d87 into GridTools:main Jan 10, 2025
31 checks passed
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.

2 participants