Matrix Implementations and tests. Update pom.xml for Java 17; add Breeze module and new matrix classes #61
Conversation
Worked with AntonGrubisic
WalkthroughThis update revises the build configuration in the Changes
Sequence Diagram(s)sequenceDiagram
participant T as Test Suite
participant M3 as Matrix3x3
participant M4 as Matrix4x4
participant Ops as MatrixOps
T->>M3: Create 3x3 matrix instance
M3->>M3: Validate dimensions & perform operations
T->>M3: Call inverse()
Note right of M3: Compute determinant and cofactors
M3->>T: Return inverse matrix or throw exception
T->>M4: Create 4x4 matrix instance
M4->>M4: Validate and perform multiplication, transpose, and inversion
M4->>T: Return results
T->>Ops: Request transformation or decomposition (scaling, rotation, LU/QR)
Ops->>M3/M4: Utilize underlying matrix operations
Ops->>T: Return transformed or decomposed matrices
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
src/main/java/org/fungover/breeze/Matrix/Matrix4x4.java (4)
47-59: Matrix multiplication logic appears correct.
Consider whether single-precision float is appropriate for your application or if double-precision is needed for numerical stability.
61-67: Determinant calculation uses cofactor expansion properly.
Math.pow(-1, col)works, but a direct sign check (e.g.(col % 2 == 0) ? 1 : -1) might be slightly faster.
107-137: Consider reducing or redirecting debug output.
Printing the adjugate and inverse in a production environment could be excessive. Also consider using a configurable tolerance for near-singular matrices.
139-149: Matrix printing is straightforward.
For production, consider using logging or returning a formatted string instead of printing directly.src/main/java/org/fungover/breeze/Matrix/Matrix3x3.java (2)
48-60: Matrix multiplication for 3x3 is correct.
If high numerical accuracy is required, consider double precision.
81-137: Inverse method includes helpful debug prints.
Consider using a logger or removing prints in production code. Also be cautious of numerical stability if determinant is very small but nonzero.src/main/java/org/fungover/breeze/Matrix/MatrixOps.java (1)
129-174: QR decomposition uses naive Gram-Schmidt.
Be aware of potential numerical instability for certain matrices. Consider Householder or modified Gram-Schmidt if needed.src/test/java/org/fungover/breeze/Matrix/Matrix3x3Test.java (3)
13-41: Add edge case tests for constructor validation.The constructor validation tests could be more comprehensive. Consider adding tests for:
nullinput- Empty array
- Ragged arrays (rows with different lengths)
84-89: Consider using parameterized tests for inverse matrix validation.The inverse matrix test could be more robust by using parameterized tests with multiple test cases.
Example using JUnit's
@ParameterizedTest:@ParameterizedTest @MethodSource("provideMatricesForInverse") void testInverse(float[][] input, float[][] expectedInverse) { Matrix3x3 matrix = new Matrix3x3(input); Matrix3x3 result = matrix.inverse(); assertArrayEquals(expectedInverse, result.getMatrix(), 0.0001f); } private static Stream<Arguments> provideMatricesForInverse() { return Stream.of( Arguments.of( new float[][]{{4, 7, 2}, {3, 6, 1}, {2, 5, 3}}, new float[][]{{1.4444444f, -1.2222222f, -0.5555556f}, {-0.7777778f, 0.8888889f, 0.22222222f}, {0.33333334f, 0.22222222f, 0.33333334f}} ), // Add more test cases here ); }Also applies to: 156-158
173-193: Add boundary tests for vector transformation.The vector transformation test could include more edge cases:
- Vectors with extreme values
- Vectors with all zeros
- Vectors with NaN or Infinity values
src/test/java/org/fungover/breeze/Matrix/Matrix4x4Test.java (1)
7-7: Consider using more specific assertions.The test class uses generic JUnit assertions. Consider using AssertJ's fluent assertions for more readable and specific matrix assertions.
-import static org.junit.jupiter.api.Assertions.*; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.within;src/test/java/org/fungover/breeze/Matrix/MatrixOpsTest.java (1)
13-65: Use parameterized tests for rotation tests.The rotation tests have similar structure and could be simplified using parameterized tests.
Example implementation:
@ParameterizedTest @CsvSource({ "0, 1,0,0, 0,1,0, 0,0,1", "90, 0,0,1, 0,1,0, -1,0,0", "180, -1,0,0, 0,1,0, 0,0,-1", "270, 0,0,-1, 0,1,0, 1,0,0" }) void testRotateY(float degrees, float... expected) { Matrix3x3 result = MatrixOps.rotateY3x3((float) Math.toRadians(degrees)); float[][] expectedMatrix = new float[3][3]; for (int i = 0; i < 3; i++) { for (int j = 0; j < 3; j++) { expectedMatrix[i][j] = expected[i * 3 + j]; } } assertMatrixEquals(expectedMatrix, result.getMatrix(), 1e-6f); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pom.xml(4 hunks)src/main/java/module-info.java(1 hunks)src/main/java/org/fungover/breeze/Matrix/Matrix3x3.java(1 hunks)src/main/java/org/fungover/breeze/Matrix/Matrix4x4.java(1 hunks)src/main/java/org/fungover/breeze/Matrix/MatrixOps.java(1 hunks)src/test/java/org/fungover/breeze/Matrix/Matrix3x3Test.java(1 hunks)src/test/java/org/fungover/breeze/Matrix/Matrix4x4Test.java(1 hunks)src/test/java/org/fungover/breeze/Matrix/MatrixOpsTest.java(1 hunks)
🔇 Additional comments (21)
src/main/java/org/fungover/breeze/Matrix/Matrix4x4.java (5)
27-32: Constructor logic is correct.
Checks for exactly 4x4 dimensions and throws an exception otherwise, which is good for enforcing correct usage.
34-36: Getter method for internal matrix is straightforward.
No issues observed.
38-45: Identity matrix creation is correct.
Implementation follows the standard identity definition.
69-93: Cofactor computation is properly implemented.
Constructing the 3x3 minor and applying the checkerboard sign is correct.
95-105: Transpose method logic is correct.
Swaps rows and columns as expected.src/main/java/org/fungover/breeze/Matrix/Matrix3x3.java (6)
32-37: Constructor ensures 3x3 dimensions.
This validation helps prevent incorrect usage.
39-45: Identity matrix creation is standard.
No issues found.
63-71: Transpose method is correct.
Properly switches rows and columns.
73-79: Determinant computation is standard for 3x3.
No correctness issues observed.
141-150: Vector transformation logic is straightforward.
Correct multiplication of vector by matrix.
154-156: Getter method returns internal matrix.
Implementation is concise and clear.src/main/java/org/fungover/breeze/Matrix/MatrixOps.java (8)
16-23: scale3x3 method is correct.
Implements standard scaling transformation in 3D.
25-34: rotateX3x3 is correctly implemented.
Uses sine/cosine for rotation about the X-axis.
36-45: rotateY3x3 is standard for Y-axis rotation.
No issues identified.
47-56: rotateZ3x3 follows typical rotation matrix conventions.
Looks good.
58-65: translate3x3 implements a 2D translation in homogeneous coordinates.
No concerns.
69-77: scale4x4 is standard for uniform/directional scaling in 4D.
Implementation is correct.
79-89: rotateX4x4 is correct for 3D rotation around X-axis in 4D homogeneous space.
No issues found.
92-127: LU decomposition (Doolittle style) lacks pivot checks.
Division by zero could occur ifU[i][i]is zero. Consider partial pivoting or singular checks.src/main/java/module-info.java (1)
1-3:✅ Verification successful
Verify the use of incubator module.
The
jdk.incubator.vectormodule is an incubating API. Please ensure you understand the implications:
- It may change incompatibly or be removed in future JDK versions.
- It requires explicit command-line flags to use.
🌐 Web query:
What is the current status of the jdk.incubator.vector module? Is it still incubating or has it been standardized?💡 Result:
The
jdk.incubator.vectormodule remains in incubation as of JDK 24 (February 2025), continuing its experimental status since its introduction in JDK 16 through nine consecutive incubation phases[3][10]. Here's the detailed breakdown:Current Status
- Still incubating: The API has not yet graduated to standard Java SE despite nine rounds of incubation (JDK 16 through JDK 24)[3][10].
- Dependency on Project Valhalla: Final standardization awaits Valhalla's value objects and generic specialization features, which are prerequisites for adapting the Vector API[3][5][10].
Key Reasons for Continued Incubation
Valhalla Integration Requirements
- Needs Valhalla's value classes to replace current value-based vector implementations[5][10]
- Requires generic specialization for primitive types like
intinstead of boxedInteger[10]Implementation Progress
- Maintains dual implementations:
- Fallback Java implementation for basic functionality
- C2 compiler intrinsics for hardware acceleration[10]
- Recent focus on float16 support and enhanced loop optimization[10]
Historical Context
JDK Version Incubation Phase Key Development 16 First Initial incubator module[1][4] 17-22 Second-Seventh Bug fixes & performance tweaks[5] 23 Eighth Preparation for Valhalla alignment[10] 24 Ninth Continued waiting for Valhalla previews[3] Developers can still use the API by explicitly enabling the incubator module with VM flags like
--add-modules=jdk.incubator.vector[8], though its unstable status means future changes are likely before standardization.Citations:
- 1: https://alexklibisz.com/2023/02/25/accelerating-vector-operations-jvm-jdk-incubator-vector-project-panama.html
- 2: https://docs.oracle.com/en/java/javase/19/docs/api/jdk.incubator.vector/module-summary.html
- 3: https://www.infoq.com/news/2025/02/java-24-so-far/
- 4: https://openjdk.org/jeps/338
- 5: https://openjdk.org/jeps/438
- 6: https://www.youtube.com/watch?v=CgrLdTZpR0c
- 7: https://docs.oracle.com/en/java/javase/17/docs/api/jdk.incubator.vector/jdk/incubator/vector/package-summary.html
- 8: Add --add-modules=jdk.incubator.vector to the guide YouHaveTrouble/minecraft-optimization#71
- 9: https://download.java.net/java/early_access/genzgc/docs/api/jdk.incubator.vector/module-summary.html
- 10: https://openjdk.org/jeps/469
Subject: Usage of jdk.incubator.vector Verified as Intended
The code in
src/main/java/module-info.javacorrectly requires the incubating vector API. As confirmed by recent information for JDK 24 (February 2025):
- The
jdk.incubator.vectormodule remains incubating and has done so consistently since JDK 16.- It continues to require explicit command-line flags (e.g.,
--add-modules=jdk.incubator.vector) for enabling its use.- Developers should remain aware that, as an experimental API, it might change incompatibly or be removed in future JDK releases—pending further integration with enhancements from Project Valhalla.
No changes are needed in the current code snippet:
module breeze { requires jdk.incubator.vector; }pom.xml (1)
20-24: LGTM! Good choice of testing dependencies.The update to
junit-jupiter-apiand addition ofassertj-corewith recent versions are good choices for the test framework.Also applies to: 26-30
| public void testMatrixInverseAndSingularMatrix1() { | ||
| // ✅ Test 1: Invertible Matrix | ||
| float[][] matrixValues = { | ||
| {4, 7, 2, 3}, | ||
| {3, 6, 1, 2}, | ||
| {2, 5, 3, 1}, | ||
| {1, 8, 7, 6} | ||
| }; | ||
|
|
||
| // Expected inverse (precomputed) | ||
| float[][] expectedInverse = { | ||
| { 1.0000f, -1.0000f, 0.9091f, -0.1818f }, | ||
| {-0.6667f, 0.8333f, 0.0606f, 0.0455f }, | ||
| { 0.3333f, -0.6667f, 0.3333f, -0.0000f }, | ||
| { 0.3333f, -0.1667f, -0.4848f, 0.1364f } | ||
| }; | ||
|
|
||
| // Create matrix object | ||
| Matrix4x4 matrix = new Matrix4x4(matrixValues); | ||
| Matrix4x4 inverseMatrix = matrix.inverse(); | ||
| float[][] result = inverseMatrix.getMatrix(); | ||
|
|
||
| // Assert each element of the inverse matrix | ||
| for (int i = 0; i < 4; i++) { | ||
| assertArrayEquals(expectedInverse[i], result[i], DELTA, "Row " + i + " in inverse matrix is incorrect."); | ||
| } | ||
|
|
||
| // ✅ Test 2: Singular Matrix (Non-invertible) | ||
| float[][] singularMatrixValues = { | ||
| {1, 2, 3, 4}, | ||
| {2, 4, 6, 8}, // This row is a multiple of the first row | ||
| {3, 6, 9, 12}, | ||
| {4, 8, 12, 16} | ||
| }; | ||
|
|
||
| Matrix4x4 singularMatrix = new Matrix4x4(singularMatrixValues); | ||
|
|
||
| // Ensure the method throws an exception when trying to invert a singular matrix | ||
| assertThrows(ArithmeticException.class, singularMatrix::inverse, "Should throw exception for singular matrix"); | ||
| } |
There was a problem hiding this comment.
Remove duplicate test method.
The testMatrixInverseAndSingularMatrix method appears twice with slightly different implementations. This creates maintenance issues and confusion.
Merge these methods into a single comprehensive test:
- Keep the first implementation (lines 84-123)
- Remove the duplicate implementation (lines 158-198)
Also applies to: 158-198
| private void printMatrix(float[][] matrix) { | ||
| for (float[] row : matrix) { | ||
| System.out.println(Arrays.toString(row)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused debug method.
The printMatrix method appears to be used for debugging and should be removed from the test class.
| public void testLUDecomposition() { | ||
| Matrix4x4 mat = new Matrix4x4(new float[][]{ | ||
| {4, -2, 1, 3}, | ||
| {3, 6, 2, 1}, | ||
| {1, -1, 3, 5}, | ||
| {2, 1, 3, 4} | ||
| }); | ||
|
|
||
| Matrix4x4[] lu = MatrixOps.luDecomposition4x4(mat); | ||
| Matrix4x4 L = lu[1]; | ||
| Matrix4x4 U = lu[0]; | ||
|
|
||
| // Expected L matrix | ||
| float[][] expectedL = { | ||
| {4.0f, -2.0f, 1.0f, 3.0f}, | ||
| {0.0f, 7.5f, 1.25f, -1.25f}, | ||
| {0.0f, 0.0f, 2.8333333f, 4.1666665f}, | ||
| {0.0f, 0.0f, 0.0f, -0.35294104f} | ||
| }; | ||
|
|
||
| // Expected U matrix | ||
| float[][] expectedU = { | ||
|
|
||
| {1.0f, 0.0f, 0.0f, 0.0f}, | ||
| {0.75f, 1.0f, 0.0f, 0.0f}, | ||
| {0.25f, -0.06666667f, 1.0f, 0.0f}, | ||
| {0.5f, 0.26666668f, 0.76470584f, 1.0f} | ||
| }; | ||
|
|
||
|
|
||
|
|
||
| // Flatten matrices for comparison | ||
| float[] expectedLFlattened = new float[16]; | ||
| float[] actualLFlattened = new float[16]; | ||
|
|
||
| float[] expectedUFlattened = new float[16]; | ||
| float[] actualUFlattened = new float[16]; | ||
|
|
||
| // Flatten expected L matrix | ||
| int index = 0; | ||
| for (float[] row : expectedL) { | ||
| for (float value : row) { | ||
| expectedLFlattened[index++] = value; | ||
| } | ||
| } | ||
|
|
||
| // Flatten actual L matrix | ||
| index = 0; | ||
| for (float[] row : L.getMatrix()) { | ||
| for (float value : row) { | ||
| actualLFlattened[index++] = value; | ||
| } | ||
| } | ||
|
|
||
| // Flatten expected U matrix | ||
| index = 0; | ||
| for (float[] row : expectedU) { | ||
| for (float value : row) { | ||
| expectedUFlattened[index++] = value; | ||
| } | ||
| } | ||
|
|
||
| // Flatten actual U matrix | ||
| index = 0; | ||
| for (float[] row : U.getMatrix()) { | ||
| for (float value : row) { | ||
| actualUFlattened[index++] = value; | ||
| } | ||
| } | ||
|
|
||
| // Compare flattened matrices with a delta for floating-point precision | ||
| for (int i = 0; i < expectedLFlattened.length; i++) { | ||
| assertEquals(expectedLFlattened[i], actualLFlattened[i], 0.0001f); // Use a smaller tolerance | ||
| } | ||
|
|
||
| for (int i = 0; i < expectedUFlattened.length; i++) { | ||
| assertEquals(expectedUFlattened[i], actualUFlattened[i], 0.0001f); // Use a smaller tolerance | ||
|
|
||
| } | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for decomposition properties.
The LU decomposition test should validate that L*U equals the original matrix.
Add this validation:
// Validate that L*U equals the original matrix
float[][] product = multiplyMatrices(L.getMatrix(), U.getMatrix());
for (int i = 0; i < 4; i++) {
assertArrayEquals(mat.getMatrix()[i], product[i], 0.0001f);
}| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <argLine> | ||
| --add-modules jdk.incubator.vector | ||
| </argLine> | ||
| </configuration> | ||
| </plugin> |
There was a problem hiding this comment.
Specify version for maven-surefire-plugin.
The plugin version should be explicitly specified to ensure build reproducibility.
Apply this diff to add the version:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
+ <version>3.2.5</version>
<configuration>
<argLine>
--add-modules jdk.incubator.vector
</argLine>
</configuration>
</plugin>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-surefire-plugin</artifactId> | |
| <configuration> | |
| <argLine> | |
| --add-modules jdk.incubator.vector | |
| </argLine> | |
| </configuration> | |
| </plugin> | |
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-surefire-plugin</artifactId> | |
| <version>3.2.5</version> | |
| <configuration> | |
| <argLine> | |
| --add-modules jdk.incubator.vector | |
| </argLine> | |
| </configuration> | |
| </plugin> |
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| <version>3.8.0</version> | ||
| <configuration> | ||
| <source>17</source> <!-- Ensure this matches your Java version --> | ||
| <target>17</target> <!-- Ensure this matches your Java version --> | ||
| <compilerArgs> | ||
| <arg>--add-modules</arg> | ||
| <arg>jdk.incubator.vector</arg> | ||
| </compilerArgs> | ||
| </configuration> | ||
| </plugin> |
There was a problem hiding this comment.
Update maven-compiler-plugin configuration.
Several issues need to be addressed:
- The plugin version 3.8.0 is outdated. Latest version should be used.
- There's a version mismatch: properties specify Java 23 but compiler config uses Java 17.
- The source/target configuration is redundant when using maven.compiler.release property.
Apply this diff to fix the issues:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
- <version>3.8.0</version>
+ <version>3.12.1</version>
<configuration>
- <source>17</source> <!-- Ensure this matches your Java version -->
- <target>17</target> <!-- Ensure this matches your Java version -->
<compilerArgs>
<arg>--add-modules</arg>
<arg>jdk.incubator.vector</arg>
</compilerArgs>
</configuration>
</plugin>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-compiler-plugin</artifactId> | |
| <version>3.8.0</version> | |
| <configuration> | |
| <source>17</source> <!-- Ensure this matches your Java version --> | |
| <target>17</target> <!-- Ensure this matches your Java version --> | |
| <compilerArgs> | |
| <arg>--add-modules</arg> | |
| <arg>jdk.incubator.vector</arg> | |
| </compilerArgs> | |
| </configuration> | |
| </plugin> | |
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-compiler-plugin</artifactId> | |
| <version>3.12.1</version> | |
| <configuration> | |
| <compilerArgs> | |
| <arg>--add-modules</arg> | |
| <arg>jdk.incubator.vector</arg> | |
| </compilerArgs> | |
| </configuration> | |
| </plugin> |
There was a problem hiding this comment.
Looks like there are alot of changes to the pom.xml file including setting java version to 17? Since we are building the rest of the package using java 23 this shouldn't be changed unless not possible to use newer versions which seems odd?
I assume that the only things that needs changing in pom.xml are
<compilerArgs>
<arg>--add-modules</arg>
<arg>jdk.incubator.vector</arg>
</compilerArgs>Maybe we can set it with something like:
<properties>
<maven.compiler.release>23</maven.compiler.release>
<jdk.incubator.vector.arg>--add-modules</jdk.incubator.vector.arg>
<jdk.incubator.vector.module>jdk.incubator.vector</jdk.incubator.vector.module>
</properties>
<build>
<plugins>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.13.0</version>
<configuration>
<release>${maven.compiler.release}</release>
<compilerArgs>
<arg>${jdk.incubator.vector.arg}</arg>
<arg>${jdk.incubator.vector.module}</arg>
</compilerArgs>
</configuration>
</plugin>
</plugins>
</build>Do we need more than this as changes in pom.xml?
There was a problem hiding this comment.
@kappsegla Your suggestion is excellent! Yes, we should maintain Java 23 and use properties for better maintainability. We also need to update the maven-surefire-plugin to use the same properties for consistency:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
<argLine>
${jdk.incubator.vector.arg} ${jdk.incubator.vector.module}
</argLine>
</configuration>
</plugin>These are the only changes needed in the pom.xml for this PR.
✏️ Learnings added
Learnt from: kappsegla
PR: fungover/breeze#61
File: pom.xml:72-84
Timestamp: 2025-02-14T12:03:47.743Z
Learning: In Maven configuration, prefer using properties for module configuration (e.g., `jdk.incubator.vector.arg` and `jdk.incubator.vector.module`) to maintain consistency across plugins and improve maintainability. Avoid using redundant source/target configuration when maven.compiler.release is already set.
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.
There was a problem hiding this comment.
I have an even better suggestion. Since we have a module-info.java file:
module breeze { requires jdk.incubator.vector; }
We don't need to change anything in the pom.xml and adding argLines. We should instead revert pom.xml to its unedited state and it works perfectly without it.
There was a problem hiding this comment.
@kappsegla Excellent point! You're absolutely right. Using module-info.java is the cleaner and more idiomatic way to handle module dependencies in Java. We should indeed revert the pom.xml changes and rely on the proper module declaration. This is a much better approach.
🐰 thoughtfully nods in agreement
✏️ Learnings added
Learnt from: kappsegla
PR: fungover/breeze#61
File: pom.xml:72-84
Timestamp: 2025-02-14T12:49:06.224Z
Learning: When using Java modules, prefer declaring module dependencies in module-info.java (e.g., `requires jdk.incubator.vector;`) instead of configuring them through Maven. This is the proper way to handle module dependencies in Java 9+ and eliminates the need for additional Maven configuration.
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.
Worked with AntonGrubisic
|
There was a problem hiding this comment.
Since we have a module-info.java file importing needed modules we don't need to change anything in pom.xml. Revert to previous version.
| @@ -0,0 +1,3 @@ | |||
| module breeze { | |||
| requires jdk.incubator.vector; | |||
| } | |||
There was a problem hiding this comment.
Missing new line at the end. That's why there is a red circle with a - in it.
When adding this file to our project it becomes modularized.
Non-Modularized Applications: Continue to use the library without any changes.
Modularized Applications: Need to explicitly declare dependencies on the new module. Is this a major change?
| @@ -0,0 +1,3 @@ | |||
| module breeze { | |||
| requires jdk.incubator.vector; | |||
There was a problem hiding this comment.
If a modularized application sees our module it will see no packages or classes as it is. We need to export any packages that we want to be open to the rest of the world. Maybe this should be its own issue? From what I can see there are no usage of the incubator vector api yet? So it isn't needed at all right now?
| @@ -0,0 +1,3 @@ | |||
| module breeze { | |||
There was a problem hiding this comment.
Suggest using the name org.fungover.breeze as a better module name.
| @@ -0,0 +1,157 @@ | |||
| package org.fungover.breeze.Matrix; | |||
There was a problem hiding this comment.
Package names should be all lowercase for best compatibility between different operating systems file system.
| // Print Cofactor Matrix | ||
| System.out.println("Cofactor Matrix:"); | ||
| for (float[] row : cofactorMatrix) { | ||
| System.out.println(Arrays.toString(row)); | ||
| } |
There was a problem hiding this comment.
We don't want any printing in our library methods. Might be usefull for debugging and in that case we can maybe wrap it in an if statement that checks on some variable like.
private static final boolean DEBUG_MODE = false;
if( DEBUG_MODE) {
...
}
The Java compiler (javac) employs a feature called constant folding. When you declare a private static final boolean variable and set it to a constant value, the compiler treats it as a compile-time constant. If the variable is used in a conditional statement like if (DEBUG_MODE) { ... }, and the condition is always false, the compiler will optimize the code by removing the entire block inside the if statement during compilation.
Summary by CodeRabbit
New Features
Chores