Skip to content

Scale linear system of equations in plane_stress and skip test if incompatible system#3884

Merged
timothy-nunn merged 4 commits intomainfrom
stabalise-plane-stress
Nov 27, 2025
Merged

Scale linear system of equations in plane_stress and skip test if incompatible system#3884
timothy-nunn merged 4 commits intomainfrom
stabalise-plane-stress

Conversation

@timothy-nunn
Copy link
Copy Markdown
Collaborator

Sadly, #3856 was short lived and enabling numpy>=2 re-introduced the issues observed in #3027.

I have been unable to find an acceptably fast solution that will ensure parity on different systems when solving $Ax=b$ in the plane_stress function. The issue arises (I believe) because the matrix aa is ill-conditioned (on the order of $10^{12}$).

The first thing this PR introduces is a row-wise scaling of the aa matrix and bb vector such that the largest value on a row of aa will be 1.0. This reduces the condition of aa significantly (to be on the order of $10^{3}$). However, one of the plane_stress tests still failed on Mac, the other two passed.

The second thing I have done is skip this test if the system is deemed incompatible (we actually already had a function for this that I wrote back in 2021). This means that the plane_stress tests are skipped on incompatible systems (e.g. Mac, Windows, CSD3) and will run when we think it should pass (on Ubuntu VMs and the CI). Maybe the reviewer could check that these tests don't get skipped on WSL/their Ubuntu laptop.

I have also added a warning to the installation page to ensure this issue is abundantly obvious.

image

@timothy-nunn timothy-nunn self-assigned this Oct 3, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 3, 2025

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 46.04%. Comparing base (e5f34c1) to head (6ef31be).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
process/tf_coil.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3884      +/-   ##
==========================================
- Coverage   46.04%   46.04%   -0.01%     
==========================================
  Files         123      123              
  Lines       28962    28965       +3     
==========================================
  Hits        13336    13336              
- Misses      15626    15629       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@timothy-nunn
Copy link
Copy Markdown
Collaborator Author

@chris-ashe checked on WSL and no unit tests are skipped

@timothy-nunn timothy-nunn force-pushed the stabalise-plane-stress branch from eaf954e to 9a920d6 Compare October 6, 2025 13:29
@timothy-nunn
Copy link
Copy Markdown
Collaborator Author

timothy-nunn commented Oct 8, 2025

Correctly skips on CSD3 (where #3027 was first observed)

@timothy-nunn timothy-nunn force-pushed the stabalise-plane-stress branch from e0a8619 to 1394fab Compare November 25, 2025 15:54
Copy link
Copy Markdown
Contributor

@jonmaddock jonmaddock left a comment

Choose a reason for hiding this comment

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

I approved of the changes: just a couple of comments for consideration. Well done of finding the root cause of these well-above-floating point errors from a single calculation between different systems!

Comment thread process/tf_coil.py Outdated
# change the solution provided each element of a given row is scaled by the same scalar
# and the corresponding entry in bb is also scaled the same amount.
row_scale = np.max(np.abs(aa), axis=1)
aa /= np.repeat(row_scale[:, np.newaxis], aa.shape[0], axis=1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be clearer to broadcast row_scale here?

Comment thread process/tf_coil.py
# Here, we scale aa such that the largest element on a given row is 1.0. This does not
# change the solution provided each element of a given row is scaled by the same scalar
# and the corresponding entry in bb is also scaled the same amount.
row_scale = np.max(np.abs(aa), axis=1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This works nicely: thank you for helping me understand ill-posed problems and condition number. I can now see that this could cause floating-point errors to lead to (well-) above floating point errors in the solution for $x$ even in a single iteration.

It seems strange to me that there is apparently no convenience function for performing this in numpy, for example: (I see there's a cond() function). I believe this falls into the class of regularisation techniques with various other methods, but this custom solution is certainly the simplest and works fine. I don't see any reason to do any different.

@timothy-nunn timothy-nunn merged commit 26bfc59 into main Nov 27, 2025
18 checks passed
@timothy-nunn timothy-nunn deleted the stabalise-plane-stress branch November 27, 2025 11:46
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