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

Another perp fix #362

Merged
merged 26 commits into from
Apr 24, 2023
Merged

Another perp fix #362

merged 26 commits into from
Apr 24, 2023

Conversation

jshipton
Copy link
Contributor

@jshipton jshipton commented Apr 14, 2023

This fixes the issue that ufl.replace doesn't replace perp(u) on the plane (because it doesn't see the relationship between [u0, u1] and [-u1, u0]). The fix is to label the terms for which this is a problem and then the Gusto replace functions pass perp(u) to ufl.replace.

This includes a test comparing to benchmarked known good values.

This PR also includes the edits introduced in #325. That PR refactors the logic in replace_{test,trial,subject} for building the dictionary to pass to ufl.replace. This simplifies the logic a bit (most type checking is deferred to ufl instead of being carried out by gusto), and allows it to be share between the three functions. It also adds the ability to pass an index to replace_{test,trial} when the new and/or old value is from a mixed space, which was previously only possible with replace_subject.

JHopeCollins and others added 25 commits January 13, 2023 14:45
Copy link
Contributor

@tommbendall tommbendall 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. Just one minor thing about adding a comment.

I think it would also be good to update the PR name and description to say that this also now includes all the changes to the replace_* routines.

gusto/labels.py Show resolved Hide resolved
unit-tests/fml_tests/test_replace_subject.py Show resolved Hide resolved
unit-tests/fml_tests/test_replace_perp.py Show resolved Hide resolved
gusto/labels.py Show resolved Hide resolved
@jshipton
Copy link
Contributor Author

@JHopeCollins are you able to edit the PR description to include a bit about what changed in your branch?

@JHopeCollins
Copy link
Collaborator

@JHopeCollins are you able to edit the PR description to include a bit about what changed in your branch?

I've added a description of those additions and a link to my original PR.

Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

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

Thanks, this also looks good now!

@tommbendall tommbendall merged commit b1c4fd5 into main Apr 24, 2023
@jshipton jshipton deleted the another_perp_fix branch March 20, 2024 10:49
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