-
Notifications
You must be signed in to change notification settings - Fork 4
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
Generalize differential element calculation to n-dims #101
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 17 17
Lines 258 260 +2
=========================================
+ Hits 258 260 +2 ☔ View full report in Codecov by Sentry. |
I don't think that adding a dependency counts as breaking change. |
I don’t think I’ve seen this documented as a strict requirement. I may be thinking back to a recent heated discussion between the Julia Slack and Discourse about license changes and how some users felt strongly that certain actions constituted a breaking change that should require a more explicit downstream acceptance. I don’t feel that strongly about it, and I don’t know how many actual users we have that might care, so it’s admittedly probably a bit of an academic question. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@JoshuaLampert Do you know a way for us to categorize some |
Also, thanks again for implementing Downgrade action! It made a good catch here. |
Yes, we can give the tests tags and filter all the tests by the tags, see https://www.julia-vscode.org/docs/stable/userguide/testitems/#Tags. Let's do that in a separate PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty cool. Thanks. I can't judge the CliffordNumbers.jl implementation because I'm not an expert in geometrical algebra, but since tests pass, I assume everything is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your explanation. I now understand what this does. While experimenting with this a bit, I came up with this alternative to handle the units. It's a bit shorter. I am not sure which version is preferable, but I thought I'll just leave it here. I you prefer the original approach I'm totally fine with that.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
I added a Also, I did some testing locally with a Update: the huge difference I was initially seeing locally seem to have been anomalous. I just ran a more thorough |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joshua Lampert <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! I'll give this one final review this evening. |
I made two changes:
|
Changes
differential
to handle geometries with arbitrary numbers of parametric dimensions.error
function into throwing more appropriate/specific error types.Box
now that this is fixed.jacobian
such that(geometry, ts) -> ::NTuple{paramdim(geometry), Meshes.Vec}