add para to parent scaling #459
Conversation
WalkthroughThe changes involve updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I think this is also missing in iga-galerkin example |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- splinepy/helpme/integrate.py (1 hunks)
- tests/helpme/test_integrate.py (1 hunks)
🔇 Additional comments (2)
tests/helpme/test_integrate.py (1)
189-194: Test implementation correctly validates volume integration for Bezier splinesThe assertion accurately verifies that integrating the
volume_functionover the Bezier spline yields the expected results. The code is correct and functions as intended.splinepy/helpme/integrate.py (1)
204-207: Adjustment of integration measure scaling is correctIncluding the scaling factor
_np.prod(_np.diff(bezier_element.control_point_bounds, axis=0))in the integration calculation properly accounts for the mapping between the parametric space and the parent element. This change effectively resolves the issue with incorrect integration of B-splines due to the omission of parametric space scaling.
| vf = np.ones((len(x), 2)) | ||
| # scale it with a factor to get a different value | ||
| vf[:, 1] = col1_factor |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify the variable name col1_factor or adjust indexing in volume_function
In the volume_function, the variable col1_factor is used to scale vf[:, 1], which corresponds to the second column (since Python uses zero-based indexing). This may cause confusion. Consider renaming the variable to col2_factor, or adjust the indexing to vf[:, 0] if you intended to scale the first column.
| # try bsplines | ||
| bspline = bezier.bspline | ||
| assert np.allclose( | ||
| [bspline.integrate.volume(), col1_factor * bspline.integrate.volume()], | ||
| bspline.integrate.parametric_function(volume_function), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding tests for rationalbezier and nurbs spline types
Currently, the test covers Bezier and B-spline types. To ensure comprehensive test coverage, consider adding similar integration tests for rationalbezier and nurbs spline types to validate the integration functionality across all spline types.
There was a problem hiding this comment.
I am guessing that the integration of a function inside the parametric domain is spline agnostic. At least for the implemented spline? The original function only iterates through the bezier view of the spline? Or is it just not implemented for the other splines?
There was a problem hiding this comment.
@clemens-fricke, thank you for the clarification. You are correct—the integration of a function inside the parametric domain is spline agnostic for the implemented splines, as it iterates over the Bezier view of the spline. Therefore, additional tests for rationalbezier and nurbs spline types may not be necessary in this context.
(•‾⌣‾•)و Curious bunny appreciates the insight!
✏️ Learnings added
Learnt from: clemens-fricke
PR: tataratat/splinepy#459
File: tests/helpme/test_integrate.py:195-200
Timestamp: 2024-10-01T08:34:23.291Z
Learning: Integration of a function inside the parametric domain is spline agnostic for the implemented splines, as it iterates over the Bezier view of the spline.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -178,15 +178,23 @@ def test_function_integration(np_rng): | |||
| col1_factor = 2 | |||
|
|
|||
| def volume_function(x): | |||
| vf = np.ones((x.shape[0], 2)) | |||
| vf = np.ones((len(x), 2)) | |||
| # scale it with a factor to get a different value | |||
| vf[:, 1] = col1_factor | |||
There was a problem hiding this comment.
Also a factor would assume a multiplication and not just the value. In this special case, the result is the same, but it might be a little weird for someone to read it and see that it just overwrites the value instead of multiplying it. So col2_value would be more accurate.
| # try bsplines | ||
| bspline = bezier.bspline | ||
| assert np.allclose( | ||
| [bspline.integrate.volume(), col1_factor * bspline.integrate.volume()], | ||
| bspline.integrate.parametric_function(volume_function), | ||
| ) |
There was a problem hiding this comment.
I am guessing that the integration of a function inside the parametric domain is spline agnostic. At least for the implemented spline? The original function only iterates through the bezier view of the spline? Or is it just not implemented for the other splines?
Overview
Integrations for bsplines weren't correct - it was missing parametric space to "parent" (or integration) element scaling.
Added corresponding test
Addressed issues
Checklists
Summary by CodeRabbit
New Features
Bug Fixes
Tests