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

slate: fix kernel argument ordering #4003

Merged
merged 3 commits into from
Feb 6, 2025
Merged

Conversation

ksagiyam
Copy link
Contributor

@ksagiyam ksagiyam commented Jan 31, 2025

Fix #4001.

Consider a slate expression:

T = A + B

where:

A = 0 * f + g
B =  f + g

Without aggressive zero simplification, we had:

A.coefficients() = (f, g)
B.coefficients() = (f, g)

which lead to T.coefficients() = (f, g, f, g)->(f, g) == B.coefficients().

Current implementation in Slate had happened to be working as B.coefficients() had been the same as T.coefficients().

With aggressive zero simplification (now merged in UFL), we have:

A.coefficients() = (g,)
B.coefficients() = (f, g)

which lead to T.coefficients() = (g, f, g)->(g, f) != B.coefficients(), and the current Slate breaks.

This PR is to fix this issue.

connorjward
connorjward previously approved these changes Jan 31, 2025
Copy link
Contributor

@connorjward connorjward left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks for taking this on.

Copy link

github-actions bot commented Jan 31, 2025

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake real8083 ran7367 passed716 skipped0 failed

Copy link

github-actions bot commented Jan 31, 2025

TestsPassed ✅Skipped ⏭️Failed ❌
Firedrake complex8111 ran6561 passed1550 skipped0 failed

@pbrubeck
Copy link
Contributor

pbrubeck commented Jan 31, 2025

Well done @ksagiyam! This is probably the Coefficient analogue to what had to be done when Constants are present in a mixed problem but not on every subblock: 6078f93

pbrubeck
pbrubeck previously approved these changes Feb 5, 2025
Copy link
Contributor

@pbrubeck pbrubeck left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@JHopeCollins JHopeCollins left a comment

Choose a reason for hiding this comment

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

If we leave this test here for now (and I do think we should until downstream testing gets a more formalised solution), then at least we can make it a bit smaller.
a) the solve doesn't need to be accurate, it just needs to run.
b) the second solve to reconstruct buoyancy doesn't test anything that isn't definitely covered elsewhere (it's a fairly simple form).

I'm happy to merge with these updates.

@ksagiyam ksagiyam force-pushed the ksagiyam/fix_slate branch 2 times, most recently from e77d9cb to 9a182b9 Compare February 6, 2025 14:13
@JHopeCollins
Copy link
Member

Approved pending CI passing.
@connorjward Do you know what this error in the checkout action means?

Error: File was unable to be removed Error: EACCES: permission denied, rmdir '/__w/firedrake/firedrake/firedrake/.github/ISSUE_TEMPLATE'

Can I just try rerunning it or does something need fixing?

@connorjward
Copy link
Contributor

Approved pending CI passing. @connorjward Do you know what this error in the checkout action means?

Error: File was unable to be removed Error: EACCES: permission denied, rmdir '/__w/firedrake/firedrake/firedrake/.github/ISSUE_TEMPLATE'

Can I just try rerunning it or does something need fixing?

I think this is a side effect of me messing about with CI for #4011. I believe I have fixed this and that it shouldn't recur. If you notice this again please let me know.

@JHopeCollins
Copy link
Member

Ok, I've set the failed job off to rerun.

@JHopeCollins JHopeCollins enabled auto-merge (squash) February 6, 2025 15:09
@JHopeCollins JHopeCollins merged commit 9650136 into master Feb 6, 2025
20 checks passed
@JHopeCollins JHopeCollins deleted the ksagiyam/fix_slate branch February 6, 2025 16:02
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.

BUG: linear solver fails if fields are initialised in a different order
6 participants