Skip to content

Implement rewrite validator #55

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

Merged
merged 63 commits into from
Jul 11, 2025
Merged

Conversation

serenity4
Copy link
Member

@serenity4 serenity4 commented Jun 20, 2025

Closes #47.

Introduces a compute_residual_vectors function which returns a pair of residuals, one corresponding to the unoptimized pipeline (no tearing, no scheduling), and one corresponding to the standard (optimized) pipeline.

Tests were added in test/validation.jl to ensure we give the same residuals in both cases, with a few tests adapted from IPO tests. The thermal fluid benchmark currently gives inconsistent results for both, and will require fixing; it may be that the unoptimized pipeline or its residual mapping is incorrect, or that we have correctness issues with the current codegen, or both. At least we have as a target to make both results match and then we'll be more confident that our codegen is sound.

Support for the ODE mode was not added in this PR, but being able to have validation tests for the DAE mode should help a long way already. We can have a follow-up PR to address this if/when we deem it important.

This PR is based on #54, for a clean view of the changes see the comparison against that PR.

serenity4 added 30 commits June 4, 2025 16:39
Not to confuse it with the nonlocal insertion
@serenity4 serenity4 marked this pull request as ready for review July 8, 2025 16:08
@serenity4
Copy link
Member Author

Small bike-shedding: any opinions on settings.skip_optimizations = true vs settings.unoptimized = true vs settings.optimize = false (or any other naming proposal) to denote the context in which we codegen without state selection nor tearing nor scheduling? I like the latter, but that breaks the consistency where settings are false by default and setting them to true changes the default behavior.

(cc @topolarity)

@serenity4 serenity4 changed the title [WIP] Implement rewrite validator Implement rewrite validator Jul 8, 2025
@Keno
Copy link
Member

Keno commented Jul 10, 2025

I've merged the other PR, which has made a bit of a mess of the history of this one. Maybe easiest to just squash this down and rebase, but I'll let you take it as you see fit.

@Keno
Copy link
Member

Keno commented Jul 10, 2025

I still don't understand the infer_residual_sign thing. I think it's possible that that's just a mistake in how we're codegen'ing the residuals for the optimized DAE somewhere (e.g. in the mass matrix or the implicit diff relationships) where I wasn't being very careful about the order of the operands (because it doesn't really matter to the solver). Can you take another look at that?

@Keno
Copy link
Member

Keno commented Jul 10, 2025

I am a little bit worried that we are doing the structural analysis on differing IR. The validation only works if we get the exact same equation/variable numbers so if the IR differs that's not guaranteed. I need to take a closer look at that, but I don't think it needs to block merging.

@Keno
Copy link
Member

Keno commented Jul 10, 2025

Looks good in general, I think there's some further tweaks to be done on the rewrite logic, but I think it'd make sense to get this merged first, so let's figure out the residual sign thing, get this rebased and merged, and then we can tackle that.

@serenity4
Copy link
Member Author

serenity4 commented Jul 10, 2025

I'd be happy to get rid of infer_residual_sign entirely. Shall I consider that all residuals should have the same sign as the defining equation, e.g. for ddt(x) := 1 have always!(ddt(x)) yield 1 and always!(-ddt(x)) yield -1? That'd make residual comparison with the unoptimized pipeline much easier.

The validation only works if we get the exact same equation/variable numbers so if the IR differs that's not guaranteed.

Yes, so far I tried to make things correspond but we should have robust tests/guarantees for that. In particular, even if it was 100% consistent with the current implementation, future changes might break that. We can work on that first after merging this initial implementation.

@serenity4
Copy link
Member Author

serenity4 commented Jul 10, 2025

I've merged the other PR, which has made a bit of a mess of the history of this one. Maybe easiest to just squash this down and rebase, but I'll let you take it as you see fit.

That's probably because I cherry-picked instead of merged the state of the other branch while working on this one. I resolved conflicts with main, but it still contains a longer history than it needs. I'd avoid rebasing the 65 commits to make it shorter, I think it's fine here and I'll just avoid this process in the future.

@Keno
Copy link
Member

Keno commented Jul 10, 2025

I'd be happy to get rid of infer_residual_sign entirely. Shall I consider that all residuals should have the same sign as the defining equation, e.g. for ddt(x) := 1 have always!(ddt(x)) yield 1 and always!(-ddt(x)) yield -1? That'd make residual comparison with the unoptimized pipeline much easier.

Yes, I think we should try to maintain that invariant. As I said, I possibly wasn't particularly careful about it in the existing implementation, because it doesn't matter too much there, but it's good to be consistent.

@Keno
Copy link
Member

Keno commented Jul 11, 2025

Since CI is passing, I'll just merge this as is and do follow-on suggestions in a PR, so we can discuss there.

@Keno Keno merged commit 924e8c4 into JuliaComputing:main Jul 11, 2025
2 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.

Add rewrite validator
2 participants