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

Use zero instead of similar to avoid undefined behavior #346

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Jan 15, 2025

This will help prevent #283 and #301 from resurfacing for users.

@charleskawczynski charleskawczynski changed the title Use zero instead of similar to avoid UB Use zero instead of similar to avoid undefined behavior Jan 15, 2025
@charleskawczynski charleskawczynski force-pushed the ck/zeros branch 4 times, most recently from 4d66a97 to 71ed4be Compare January 15, 2025 04:12
@charleskawczynski
Copy link
Member Author

charleskawczynski commented Jan 15, 2025

The bad news is that I don't think that this PR is fixing the issue. The good news is that I did see NaNs when running the unit tests locally. It's surprising, but then again, I don't think that we actually have tests for them, and they're not failing like the docs because we don't plot in the unit tests.

I hope that this can accelerate the bug hunting.

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Jan 17, 2025

Ok, good news, I'm pretty sure that in T_imp! was the issue after all. We unfortunately can't (yet) use Base.zero for those input arrays because ClimaCore.MatrixFields.FieldMatrixWithSolver does not support Base.zero, nor does it support Base.fill!. Let's initialize the tendency for now, update ClimaCore, and then we can change CTS to require Base.zero / Base.fill! (for the next breaking release).

@charleskawczynski charleskawczynski added bugfix testing Unit and regression testing labels Jan 17, 2025
@charleskawczynski charleskawczynski force-pushed the ck/zeros branch 2 times, most recently from bf3d9c1 to e5597ed Compare January 17, 2025 16:58
test/problems.jl Outdated Show resolved Hide resolved
src/ClimaTimeSteppers.jl Outdated Show resolved Hide resolved
@Sbozzolo
Copy link
Member

Could you please run a ClimaAtmos test suite to verify that everything still works there?

I think that this change will also implicitely raise the lower compat for ClimaCore in all our packages (to the version that contains the definion of zero).

@charleskawczynski
Copy link
Member Author

It works 🙂

Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@charleskawczynski charleskawczynski merged commit 6e594dc into main Jan 22, 2025
10 checks passed
@charleskawczynski charleskawczynski deleted the ck/zeros branch January 22, 2025 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix testing Unit and regression testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants