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

-## 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

+## 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

+.. 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

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


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

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


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

-        # 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

+        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

+        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

+    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

+            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

+
    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

+        # 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

+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

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

delete this too since two-cylinder only

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

+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

+.. 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

+                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

@@ -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

+    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

+
+    # --- 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

-                #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

+    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

+    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

+                '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

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

+    "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

see comment on pyproject.toml

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

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

…pper

Update badge links

Removed pull_request branch specification from CI workflow

Fix forcing-term callback signature mismatch after I_mk caching refactor

…tions

…tion pathway mock

…uctor validation test

…ation pathway type alignment

…H into jfm-paper

JFM paper

codecov · 2026-05-28T20:45:34Z

Codecov Report

❌ Patch coverage is 98.69565% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 98.55623%. Comparing base (230a8d5) to head (c29750d).

Files with missing lines	Patch %	Lines
package/src/openflash/results.py	66.66667%	3 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##           branch-after-pr6          #8         +/-   ##
==========================================================
+ Coverage          98.38833%   98.55623%   +0.16791%     
==========================================================
  Files                    11          11                 
  Lines                  1303        1316         +13     
==========================================================
+ Hits                   1282        1297         +15     
+ Misses                   21          19          -2

Flag	Coverage Δ
unit	`98.55623% <98.69565%> (+0.16791%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

…ify boolean evaluation

…lution

Potentials

rebeccamccabe reviewed Jun 26, 2025

View reviewed changes

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

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

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

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

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 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 had a problem deploying to github-pages October 24, 2025 04:33 — with GitHub Actions Failure

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

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

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

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

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

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

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

HopeBestWorld temporarily deployed to github-pages October 24, 2025 05:09 — with GitHub Actions Inactive

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

HopeBestWorld temporarily deployed to github-pages October 24, 2025 15:22 — with GitHub Actions Inactive

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

HopeBestWorld temporarily deployed to github-pages October 24, 2025 15:38 — with GitHub Actions Inactive

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

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

HopeBestWorld and others added 20 commits May 28, 2026 00:15

fix: remove overly strict diagnostic assertions from dense blocks

3c0c47d

fix: vectorize old code comparison potential field reconstruction wra…

491d736

…pper

Merge pull request #85 from symbiotic-engineering/readme-links

33f02bc

Update badge links

Fix stale forcing cache lambda signature after I_mk refactor

0c5eaf5

Validate fix with package tests

fcbbd0c

refactor: delete obsolete dead scalar entry helper functions

62aed4e

Update CI workflow to run on pull requests from any branch

b8a1747

Removed pull_request branch specification from CI workflow

Merge remote-tracking branch 'origin/main' into jfm-paper

a1b4d5a

Merge pull request #86 from symbiotic-engineering/copilot/jfm-paper

4973dd7

Fix forcing-term callback signature mismatch after I_mk caching refactor

remove unwanted equations

574490c

test: add comprehensive coverage for constructor heaving count valida…

63c04a2

…tions

test: robustify explicit abstract property implementations for valida…

d328c2b

…tion pathway mock

test: implement clean abstract properties on mock class to fix constr…

fdd0f84

…uctor validation test

test: implement strict abstract properties on mock class to fix valid…

6a37a8f

…ation pathway type alignment

track .pkl with dvc instead of git

bb1edd0

address PR comments

ccb49c6

Merge branch 'jfm-paper' of github.com:symbiotic-engineering/OpenFLAS…

493e09b

…H into jfm-paper

add back deleted matlab files

a1ccfa0

test: isolate constructor pathways to clear line coverage gaps

ff27722

Merge pull request #84 from symbiotic-engineering/jfm-paper

dfa1a32

JFM paper

HopeBestWorld and others added 9 commits May 28, 2026 20:50

test: use clean instantiation for validation fallback test and robust…

bb3e774

…ify boolean evaluation

update gitignore

524dd4a

update gitmodules

85ba6e5

update dev python multi_equations

48e501a

update wavenumber equation

16603ca

update excitation phase equation

b9abeed

merge: resolve conflicts with main and sync submodules

76d740c

test: isolate heaving validation test to bypass global suite test pol…

c8adc03

…lution

Merge pull request #81 from symbiotic-engineering/potentials

c29750d

Potentials

		@@ -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!

codecov Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

codecov Bot commented May 28, 2026 •

edited

Loading