diff comparison from last pull request by HopeBestWorld · Pull Request #8 · symbiotic-engineering/OpenFLASH

HopeBestWorld · 2025-06-25T22:06:18Z

No description provided.

review-notebook-app · 2025-06-25T22:06:23Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

rebeccamccabe · 2025-06-25T23:22:31Z

README.md

-## Installation
+1.  **Install the package:**
+    ```bash
+    pip install git+[https://github.com/symbiotic-engineering/semi-analytical-hydro.git](https://github.com/symbiotic-engineering/semi-analytical-hydro.git)


Shouldn't this be open flash now that it's on pypi?

rebeccamccabe · 2025-06-25T23:23:43Z

README.md

+## References

-### Install from the `CS_group` Branch
+The following publications are relevant to this package:


Add citations for the JOSS paper and the JFM paper (without writing JOSS or JFM since they haven't accepted it yet), you can add our names and the title and say "in prep", see the MDOcean readme as example

rebeccamccabe · 2025-06-25T23:25:54Z

docs/constants.rst

+.. autodata:: m0
+   :annotation: = 1
+
+   Radial wave number.


Just called "wavenumber", more often denoted with k

to be consistent with your description of omega, it's "wavenumber of the incident wave in 1/m"

rebeccamccabe · 2025-06-25T23:27:25Z

docs/constants.rst

+   Radial wave number.
+
+.. autodata:: n
+   :annotation: = 3


rebeccamccabe · 2025-06-25T23:27:37Z

docs/constants.rst

+   Related to a mode or term index.
+
+.. autodata:: z
+   :annotation: = 6


rebeccamccabe · 2025-06-26T01:39:20Z

package/src/meem_engine.py

-        # Convert the complex number to a dictionary
+                    hydro_p_terms[i,j] = 0
+
+        # Now return per-mode values


we have previously used mode to refer to the harmonics of the m_k's, but here mode means the mode of motion / degree of freedom. Clarify the difference in comments, or perhaps rename mode to dof here.

rebeccamccabe · 2025-06-26T01:43:20Z

package/src/meem_engine.py

+        imag_coeffs = np.zeros(num_modes)
+
+        for mode_index in range(num_modes):
+            if mode_index < hydro_h_terms.shape[0] and mode_index < hydro_p_terms.shape[0]:


why is this conditional necessary? add comment, I don't remember why this would be the case. Would hydro_h_terms.shape[0] and hydro_p_terms.shape[0] ever be different?

rebeccamccabe · 2025-06-26T01:47:11Z

package/src/meem_engine.py

+        plt.show() # final display command

-    def run_and_store_results(self, problem_index: int, m0) -> Results:
+    def run_and_store_results(self, problem_index: int, m0_values: np.ndarray) -> 'Results': 


use from __future__ import annotations so you can type with Results (no string) even before it's defined

rebeccamccabe · 2025-06-26T01:47:39Z

package/src/meem_engine.py

+    def run_and_store_results(self, problem_index: int, m0_values: np.ndarray) -> 'Results': 
        """
-        Perform the full MEEM computation and store results in the Results class.
+        Perform the full MEEM computation for a *list of frequencies* and store results


array, not list

rebeccamccabe · 2025-06-26T01:59:45Z

package/src/meem_engine.py

+            freq_idx_in_problem = freq_to_idx.get(m0)
+            if freq_idx_in_problem is None:
+                # This should ideally not happen if m0_values are a subset of problem.frequencies
+                print(f"  Warning: m0={m0:.4f} not found in problem.frequencies. Skipping calculation.")


seems like the code structure should be tweaked so this is not a possibility: either problem.frequencies or m0_values is used but not both. My preference would be problem.frequencies

If we need to extract m0_values for some reason ie plottng, extract it from problem.frequencies so it's consistent

rebeccamccabe · 2025-06-26T02:34:19Z

package/src/multi_equations.py

+
    constant = - heaving[i] * a[i]/(2 * (h - d[i]))
    if k == 0:
        if m0 * h < 14:


use M0_H_THRESH=14 or similar constant definition so it can be changed

rebeccamccabe · 2025-06-26T02:38:06Z

package/src/multi_equations.py

+        # ensure r_array doesn't contain 0 if R_2n is called for r=0
+        return 0.5 * np.log(r_array / a[i])
+    else:
+        return besselk(0, lambda_ni(n, i, h, d) * r_array) / besselk(0, lambda_ni(n, i, h, d) * local_scale[i])


don't you need a value error if n<0 here like you have above?

rebeccamccabe · 2025-06-26T02:41:45Z

package/src/multi_equations.py

+def Z_k_e_vectorized(k, z_array, m0, h, NMK, m_k_arr): # Changed 'z' to 'z_array'
+    local_m_k = m_k(NMK, m0, h) 
+    if k == 0:
+        if m0 * h < 14:


use same const as suggested above

rebeccamccabe · 2025-06-26T13:52:30Z

package/src/coupling.py

This has wrong eqns in it, but not a worry because it will be deleted and the multi equations is correct

rebeccamccabe · 2025-06-26T13:53:08Z

package/src/equations.py

delete this too since two-cylinder only

rebeccamccabe · 2025-06-26T13:57:15Z

docs/constants.rst

+Below are the constants defined in this module:

-mass = 5.0  # Example mass in kg
+.. autodata:: g


use the constant g and rho so the user doesn't have to specify them, and can override them if they wish. Other constants like m0, geometry, etc should not go in constants, it's when they create the problem, so delete them from here.

rebeccamccabe · 2025-06-26T13:58:13Z

docs/equations.rst

+.. autofunction:: lambda_n2
+   :noindex:
+
+   Calculates the eigenvalue :math:`\lambda_n` for the middle fluid domain (Region 2).


Not relevant since deleting.

rebeccamccabe · 2025-06-26T14:04:22Z

package/src/meem_engine.py

+                if bd == 0:  # One cylinder case (Inner-Exterior directly)
+                    for n in range(N_left): # Inner harmonics (N)
+                        A[row_offset + n][col_offset + n] = (h - d[bd]) * R_1n(n, a[bd], bd, h, d, a)
+                        for m in range(M_right): # Exterior harmonics (K)


This is fixed in multi_MEEM notebook. Transfer this fix over to equations instead of engine.

rebeccamccabe · 2025-06-26T14:41:49Z

package/src/geometry.py

@@ -57,7 +57,7 @@ def make_domain_list(self) -> Dict[int, 'Domain']:
                'h': h,


don't allow h, di, and a to be independently passed domain params, but the geometry should auto create the domain objects with these properties so it's consistent.

We want Bimali's functions to discretize the slant in different ways (left, right center, etc) as methods of geometry that it uses when turning r,z coordinates (of the body, representing a curve or other non MEEM supported thing) into a list of domains

we want geometry (not problem as in the UML diagram) to have a property that is a logical array, of size num_domains x num_domains, saying whether a domain is next to each other domain.

rebeccamccabe · 2025-06-26T15:13:01Z

package/test/meem_engine_example.py

+    all_potentials_batch_data = [] # <--- NEW: List to store potential data for all frequencies/modes
+
+    print("\n--- Starting Batch Processing for Multiple Frequencies ---")
+    for i, current_m0_val in enumerate(meem_problem.frequencies):


have the for loop here not be necessary for the user to do, you can just call engine.solve and it solves all the problems in the list

rebeccamccabe · 2025-06-26T15:13:17Z

package/test/meem_engine_example.py

+
+    # --- 3. Create a MEEMProblem Instance and set multiple frequencies ---
+    meem_problem = MEEMProblem(geometry=geometry)
+    meem_problem.set_frequencies_modes(analysis_frequencies, analysis_modes)


analysis modes is redudant

rebeccamccabe · 2025-06-26T15:17:36Z

package/src/meem_engine.py

-                #AttributeError: 'Domain' object has no attribute 'r_coordinates'
-                #'r': domain.r_coordinates,
-                #'z': domain.z_coordinates
                'r': geometry_instance.r_coordinates,


Users should be able to pass a desired r/z vector for visualization (or maybe just a resolution and we create it based on the geometry)

rebeccamccabe · 2025-06-26T15:19:49Z

package/test/meem_engine_example.py

+    damping_matrix = np.array(collected_damping).reshape(num_frequencies, num_modes)
+
+    # --- 7. Instantiate Results and Store Data ---
+    results_obj = Results(geometry, frequencies_for_results, modes_for_results)


use the method that creates results for you, instead of creating it yourself

rebeccamccabe · 2025-06-26T15:39:18Z

package/test/main_test.py

+    problem_cache = engine.cache_list[problem] # Access the existing cache
+
+    if problem_cache is None:
+        print("ERROR: problem_cache is None!")


should this be an actual error?

rebeccamccabe · 2025-06-26T15:40:56Z

package/test/meem_engine_example.py

+                'data': formatted_potentials_for_batch
+            })
+
+        except np.linalg.LinAlgError as e:


error handling should happen in the meem engine, not the test

rebeccamccabe · 2025-06-26T15:47:09Z

package/test/test_problem_cache.py

Shouldn't this test, in addition to doing mocks, also test that the actual results for the non-first (cached) problem are the same as that problem evaluated without any caching (using the original _full method)

rebeccamccabe · 2025-06-26T15:49:58Z

pyproject.toml

+    "Intended Audience :: Science/Research",
+    "Topic :: Scientific/Engineering :: Physics",
+]
+dependencies = [


instead of super specific versions and all packages from an env export, this should be a minimal list of dependencies with inequalities

rebeccamccabe · 2025-06-26T15:51:08Z

requirements.txt

see comment on pyproject.toml

rebeccamccabe · 2025-06-26T19:32:37Z

docs/tutorial_walk.ipynb

great high level walkthrough, can we also include a plot of the hydro coeffs vs omega here?

The previous implementation of `run_and_store_results` was flawed. It solved for a single combined potential (all bodies heaving at once) and incorrectly tried to reshape the resulting 1xN force vector into an NxN matrix. This caused a `ValueError` for multi-body problems. This commit refactors the `run_and_store_results` method to correctly solve the N-body radiation problem: - It now loops through each frequency *and* each radiating mode `i`. - For each mode `i`, it creates a temporary `MEEMProblem` where only body `i` is heaving, and solves for its unique potential ($X_i$). - It then calculates the forces on all bodies `j` resulting from this potential, correctly building the `i`-th column of the hydrodynamic matrices ($A_{ji}$, $B_{ji}$). - This correctly assembles the full NxN added mass and damping matrices. - Potentials are now stored correctly, saving the actual computed coefficients ($C_s$) for each radiation problem instead of random data. Correspondingly, `app.py` is reverted to remove the inefficient manual-looping workaround. It now makes a single, clean call to the fixed `run_and_store_results` method.

This commit addresses two separate issues: 1. Corrects a `NameError` typo in `meem_engine.py`. - `diff_Z_n_i_vectorGized` has been renamed to `diff_Z_n_i_vectorized` inside the `calculate_velocities` method. 2. Updates `test_run_and_store_results` to correctly calculate the expected shape of the stored potentials. - The test now accounts for intermediate domains storing `2 * NMK` coefficients (for both $R_{1n}$ and $R_{2n}$ components), rather than just `NMK`. - This aligns the test's assertion with the engine's correct storage behavior, resolving the `AssertionError`.

Correct N-body radiation solve in run_and_store_results

updated package and reuploaded it to pypi

updating the streamlit app deployment

…flows/deploy-docs.yml file

the new app.html location, the conf.py edit, and the new .github/work…

installing requirements

installing open-flash

…s. Updates the CI workflow to install dependencies using pip install -e .[dev], ensuring the CI environment matches the configuration in pyproject.toml.

Updates build configurations, documentation, and CI/CD workflows to rely on pyproject.toml extras instead of the deprecated requirements.txt. - MANIFEST.in: Stop including requirements.txt. - deploy-docs.yml: Install dependencies via pip install .[dev]. - README.md: Update developer installation instructions. - installation.rst: Update troubleshooting steps.

Removes requirements.txt to eliminate redundant dependency definition…

- Updated tutorial_walk.ipynb to split the simulation into two separate cases (inner body heaving, then outer body heaving). This resolves the heaving_count <= 1 assertion error caused by setting multiple bodies to heave simultaneously before the coupling matrix is implemented. - Added package/test/test_tutorial.py to execute the tutorial notebook in CI, ensuring future regressions are caught automatically.

- Modified Results class in package/src/openflash/results.py to accept an optional modes argument. This allows pre-allocating result matrices for the full system (e.g., 2 modes) even when the problem instance only has one active mode. - Enhanced tutorial to demonstrate manual aggregation of hydrodynamic coefficients from separate mode simulations into a single Results dataset. - Added potential field visualization and domain inspection steps to the tutorial.

- Added `test_results_initialization_with_explicit_modes` to `package/test/test_results.py`. - This test verifies that passing a specific list of modes to the `Results` constructor correctly sets the `modes` attribute and updates the xarray dataset coordinates, ensuring coverage for the `if modes is not None:` initialization branch.

Fix tutorial notebook and add test

- Updated MEEMEngine.run_and_store_results to extract excitation_force and excitation_phase from the solver output and pass them to the results object. - Updated Results.store_hydrodynamic_coefficients to accept and store these new excitation metrics in the xarray dataset. - Fixed type hint in Results to allow Optional[np.ndarray] for optional arguments, resolving static analysis errors. - Fixed type hint in ProblemCache to correctly reflect that I_nm_vals is a List[np.ndarray]. - Fixed OpenFLASH.ipynb visualization by correctly importing matplotlib.patches for drawing geometry rectangles.

capture excitation force/phase and fix type hints

- Added package/test/test_matrix_verification.py to systematically generate matrix sparsity plots for all test configurations (config0-config6). - Included structural assertions (square matrix, correct dimensions based on harmonics). - Included numerical health checks (NaN/Inf detection, condition number warning, singularity check). - Implemented safe_heaving_map logic to bypass ConcentricBodyGroup assertion (single heaving body limit) during matrix assembly, as the system matrix A is independent of the heaving state.

Add matrix verification test suite for all configs

update

…_2n, Z_n, Lambda_k, Z_k, and their derivatives). This ensures that passing a Python list (as meem_engine.py does) will no longer crash the math operations

delete non vectorized functions

- Vectorized in to use block operations for potential and velocity matching, replacing O(N*M) element-wise loops. - Fixed a closure scope bug in by implementing a factory function () to strictly bind derivative vectors

Recompute

…e to capytaine

adding benchmark test comparing accuracy, memory, convergence, runtim…

rebeccamccabe reviewed Jun 26, 2025

View reviewed changes

docs/tutorial_walk.ipynb

Copy link

Contributor

rebeccamccabe Jun 26, 2025

Choose a reason for hiding this comment

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

great high level walkthrough, can we also include a plot of the hydro coeffs vs omega here?

HopeBestWorld temporarily deployed to github-pages September 18, 2025 20:47 — with GitHub Pages Inactive

HopeBestWorld temporarily deployed to github-pages October 16, 2025 23:21 — with GitHub Pages Inactive

rebeccamccabe and others added 5 commits October 20, 2025 16:19

Initialize DVC

23c1235

Set DVC remote

f5a56aa

Merge pull request #20 from symbiotic-engineering/update-app

b0e3cca

Correct N-body radiation solve in run_and_store_results

HopeBestWorld temporarily deployed to github-pages October 24, 2025 02:44 — with GitHub Pages Inactive

HopeBestWorld and others added 2 commits October 23, 2025 22:57

updated package and reuploaded it to pypi

672635b

Merge pull request #21 from symbiotic-engineering/v-0.2.5

98f8ad6

updated package and reuploaded it to pypi

HopeBestWorld temporarily deployed to github-pages October 24, 2025 03:00 — with GitHub Pages Inactive

HopeBestWorld and others added 2 commits October 23, 2025 23:47

updating the streamlit app deployment

b3902f8

Merge pull request #22 from symbiotic-engineering/app_deploy

e3df03d

updating the streamlit app deployment

HopeBestWorld temporarily deployed to github-pages October 24, 2025 03:50 — with GitHub Pages Inactive

HopeBestWorld and others added 2 commits October 24, 2025 00:14

the new app.html location, the conf.py edit, and the new .github/work…

57ac88a

…flows/deploy-docs.yml file

Merge pull request #23 from symbiotic-engineering/sphinx-deploy

c65a3d9

the new app.html location, the conf.py edit, and the new .github/work…

HopeBestWorld had a problem deploying to github-pages October 24, 2025 04:16 — with GitHub Actions Failure

HopeBestWorld temporarily deployed to github-pages October 24, 2025 04:16 — with GitHub Pages Inactive

HopeBestWorld and others added 2 commits October 24, 2025 00:19

installing requirements

ca16fa7

Merge pull request #24 from symbiotic-engineering/deploy-fix

9435b2f

installing requirements

HopeBestWorld had a problem deploying to github-pages October 24, 2025 04:22 — with GitHub Actions Failure

HopeBestWorld temporarily deployed to github-pages October 24, 2025 04:23 — with GitHub Pages Inactive

HopeBestWorld and others added 2 commits October 24, 2025 00:27

installing open-flash

7b38871

Merge pull request #25 from symbiotic-engineering/update-deploy

12d9fcb

installing open-flash

HopeBestWorld and others added 30 commits December 24, 2025 23:49

Removes requirements.txt to eliminate redundant dependency definition…

a39e281

…s. Updates the CI workflow to install dependencies using pip install -e .[dev], ensuring the CI environment matches the configuration in pyproject.toml.

Merge pull request #65 from symbiotic-engineering/refactor-ci

2c0954b

Removes requirements.txt to eliminate redundant dependency definition…

update threshold

242a1cc

add test coverage

88838ce

update thresholds

bd07fac

Merge pull request #66 from symbiotic-engineering/fix-tutorial

05d8643

Fix tutorial notebook and add test

Merge pull request #67 from symbiotic-engineering/update-notebook

2d23cd0

capture excitation force/phase and fix type hints

Merge pull request #68 from symbiotic-engineering/matrix

ffc462d

Add matrix verification test suite for all configs

update

78be147

Merge pull request #70 from symbiotic-engineering/update

736a1e6

update

delete non vectorized functions

0fa25b5

applied the np.asarray() casting to all vectorized functions (R_1n, R…

1e682ca

…_2n, Z_n, Lambda_k, Z_k, and their derivatives). This ensures that passing a Python list (as meem_engine.py does) will no longer crash the math operations

updating test

97585fd

explicitly cast inputs to float inside the vectorized functions

85107b2

Infinite Frequency limit

9e28a0a

updates the expectations for the infinite frequency case

67e278b

Merge pull request #71 from symbiotic-engineering/vectorize

8c416e5

delete non vectorized functions

Optimize MEEM matrix assembly and fix closure scope issues

13837a3

- Vectorized in to use block operations for potential and velocity matching, replacing O(N*M) element-wise loops. - Fixed a closure scope bug in by implementing a factory function () to strictly bind derivative vectors

variable name change

00c950b

optimize MEEM matrix assembly with vectorized blocks

15372cc

add explicit h5py dependency to resolve CI failure

791df5f

Merge pull request #72 from symbiotic-engineering/recompute

230a8d5

Recompute

adding benchmark test comparing accuracy, memory, convergence, runtim…

ebc8dfa

…e to capytaine

Merge pull request #73 from symbiotic-engineering/benchmark_test

3341c60

adding benchmark test comparing accuracy, memory, convergence, runtim…

		@@ -57,7 +57,7 @@ def make_domain_list(self) -> Dict[int, 'Domain']:
		'h': h,

Conversation

HopeBestWorld commented Jun 25, 2025

Uh oh!

review-notebook-app bot commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants