diff comparison from last pull request #8
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
README.md
Outdated
| ## 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) |
There was a problem hiding this comment.
Shouldn't this be open flash now that it's on pypi?
README.md
Outdated
| ## References | ||
|
|
||
| ### Install from the `CS_group` Branch | ||
| The following publications are relevant to this package: |
There was a problem hiding this comment.
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
docs/constants.rst
Outdated
| .. autodata:: m0 | ||
| :annotation: = 1 | ||
|
|
||
| Radial wave number. |
There was a problem hiding this comment.
Just called "wavenumber", more often denoted with k
There was a problem hiding this comment.
to be consistent with your description of omega, it's "wavenumber of the incident wave in 1/m"
docs/constants.rst
Outdated
| Radial wave number. | ||
|
|
||
| .. autodata:: n | ||
| :annotation: = 3 |
docs/constants.rst
Outdated
| Related to a mode or term index. | ||
|
|
||
| .. autodata:: z | ||
| :annotation: = 6 |
package/src/meem_engine.py
Outdated
| # Convert the complex number to a dictionary | ||
| hydro_p_terms[i,j] = 0 | ||
|
|
||
| # Now return per-mode values |
There was a problem hiding this comment.
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.
package/src/meem_engine.py
Outdated
| 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]: |
There was a problem hiding this comment.
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?
package/src/meem_engine.py
Outdated
| 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': |
There was a problem hiding this comment.
use from __future__ import annotations so you can type with Results (no string) even before it's defined
package/src/meem_engine.py
Outdated
| 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 |
package/src/meem_engine.py
Outdated
| 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.") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If we need to extract m0_values for some reason ie plottng, extract it from problem.frequencies so it's consistent
package/src/multi_equations.py
Outdated
|
|
||
| constant = - heaving[i] * a[i]/(2 * (h - d[i])) | ||
| if k == 0: | ||
| if m0 * h < 14: |
There was a problem hiding this comment.
use M0_H_THRESH=14 or similar constant definition so it can be changed
package/src/multi_equations.py
Outdated
| # 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]) |
There was a problem hiding this comment.
don't you need a value error if n<0 here like you have above?
package/src/multi_equations.py
Outdated
| 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: |
There was a problem hiding this comment.
use same const as suggested above
There was a problem hiding this comment.
This has wrong eqns in it, but not a worry because it will be deleted and the multi equations is correct
There was a problem hiding this comment.
delete this too since two-cylinder only
docs/constants.rst
Outdated
| Below are the constants defined in this module: | ||
|
|
||
| mass = 5.0 # Example mass in kg | ||
| .. autodata:: g |
There was a problem hiding this comment.
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.
docs/equations.rst
Outdated
| .. autofunction:: lambda_n2 | ||
| :noindex: | ||
|
|
||
| Calculates the eigenvalue :math:`\lambda_n` for the middle fluid domain (Region 2). |
There was a problem hiding this comment.
Not relevant since deleting.
package/src/meem_engine.py
Outdated
| 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) |
There was a problem hiding this comment.
This is fixed in multi_MEEM notebook. Transfer this fix over to equations instead of engine.
package/src/geometry.py
Outdated
|
|
||
| # Prepare parameters to pass to Domain | ||
| domain_params = { | ||
| 'h': h, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
package/test/meem_engine_example.py
Outdated
| 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): |
There was a problem hiding this comment.
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
package/test/meem_engine_example.py
Outdated
|
|
||
| # --- 3. Create a MEEMProblem Instance and set multiple frequencies --- | ||
| meem_problem = MEEMProblem(geometry=geometry) | ||
| meem_problem.set_frequencies_modes(analysis_frequencies, analysis_modes) |
There was a problem hiding this comment.
analysis modes is redudant
package/src/meem_engine.py
Outdated
| #AttributeError: 'Domain' object has no attribute 'r_coordinates' | ||
| #'r': domain.r_coordinates, | ||
| #'z': domain.z_coordinates | ||
| 'r': geometry_instance.r_coordinates, |
There was a problem hiding this comment.
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)
package/test/meem_engine_example.py
Outdated
| 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) |
There was a problem hiding this comment.
use the method that creates results for you, instead of creating it yourself
package/test/main_test.py
Outdated
| problem_cache = engine.cache_list[problem] # Access the existing cache | ||
|
|
||
| if problem_cache is None: | ||
| print("ERROR: problem_cache is None!") |
There was a problem hiding this comment.
should this be an actual error?
package/test/meem_engine_example.py
Outdated
| 'data': formatted_potentials_for_batch | ||
| }) | ||
|
|
||
| except np.linalg.LinAlgError as e: |
There was a problem hiding this comment.
error handling should happen in the meem engine, not the test
There was a problem hiding this comment.
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)
pyproject.toml
Outdated
| "Intended Audience :: Science/Research", | ||
| "Topic :: Scientific/Engineering :: Physics", | ||
| ] | ||
| dependencies = [ |
There was a problem hiding this comment.
instead of super specific versions and all packages from an env export, this should be a minimal list of dependencies with inequalities
There was a problem hiding this comment.
see comment on pyproject.toml
There was a problem hiding this comment.
great high level walkthrough, can we also include a plot of the hydro coeffs vs omega here?
…d put matlab files together.
- ensures the numerical output is identical to the original implementation while providing a significant performance boost for iterative solving scenarios.
- Replaces the previous iterative method for calculating spatial potentials by optimizing the function, calculate_potentials. A comprehensive test was added to validate the new function against the legacy `calculate_potentials_old` implementation. After a series of debugging steps to resolve inconsistencies in argument handling and NaN masking, the test now passes, confirming that the optimized version produces numerically identical results.
SP25 hydro group investigations and additions
added two scripts to profile in Spyder
To guide users on the end-to-end usage, a comprehensive and runnable example script was needed. updates `meem_engine_example.py`, a self-contained script that demonstrates the primary workflow for setting up, solving, and visualizing a problem. The example script covers: - Defining the physical parameters for a multi-cylinder system. - Creating the necessary `Geometry` and `MEEMProblem` objects. - Initializing the `MEEMEngine`. - Running a frequency sweep with `run_and_store_results`. - Solving for a single frequency to calculate detailed potential and velocity fields. - Visualizing the results
To clarify the setup process for a simulation, updates example script, `meem_problem_example.py`. This script serves as a tutorial for the `MEEMProblem` class, demonstrating the standard user workflow: - First, creating a `Geometry` object to define the physical system. - Then, instantiating a `MEEMProblem` with that geometry. - Finally, using the `set_frequencies_modes` method to define the specific computations to be run. This example helps illustrate the role of `MEEMProblem` that is passed to the `MEEMEngine`.
- loop over problem.modes
- updated building params method to include optional slant
Ensure docs preview artifact is downloadable via GitHub UI
updating pypi version in app_streamlit.html
Adds the necessary '_headers' file to the Sphinx build output (docs/_build/html) to enforce 'Cross-Origin-Opener-Policy: same-origin' and 'Cross-Origin-Embedder-Policy: require-corp'. This resolves a security header conflict that prevented the embedded Stlite/Streamlit application from booting correctly on GitHub Pages.
Add _headers for Cross-Origin Isolation in Stlite app
Adds an assertion to 'BodyArrangement.__init__' that limits the number of bodies marked as 'heaving' to a maximum of one. This ensures the geometry adheres to the single-body radiation problem assumption required for the initial scope of the solver.
Modifies the 'sample_problem' fixture in 'test_meem_engine.py' to mark only one body (Body 0) as heaving ([1, 0] instead of [1, 1]). This ensures the fixture adheres to the new 'BodyArrangement' assertion, allowing MEEMEngine tests to pass initialization. Updates test assertions to expect a (num_freqs, 2, 1) matrix shape instead of (num_freqs, 2, 2).
Corrects the expected shape of the 'added_mass' and 'damping' matrices in 'test_run_and_store_results'. The assertion was incorrectly expecting (num_freqs, 2 total bodies, 1 active mode); it has been changed to expect (num_freqs, 1 active mode, 1 active mode), or (3, 1, 1), aligning with the 'Results' object's design to index only by active modes (num_modes x num_modes).
Enforce single heaving body constraint
…o cap-v-openflash merge the commit. merging
- Corrected `R_1n_vectorized` logic for n=0 in regions (i > 0). Previously, it incorrectly returned a constant 0.5 (valid only for the central cylinder), ignoring the logarithmic profile `1.0 + 0.5 * log(r/a[i])`. This resolves the massive potential field discrepancies observed in Config 6. - Updated `diff_R_1n_vectorized` to correctly return `1/(2r)` for n=0 , ensuring consistency with the scalar implementation. - Added input masking to `R_2n_vectorized` and `Lambda_k_vectorized` to prevent `RuntimeWarning: invalid value encountered in divide` when evaluating Bessel functions at n=0 or k=0.
- Fix critical bug in test_hydro.py where results were normalized twice (once in utils, once in test), causing 0.00x ratios. - Update openflash_utils.py to return excitation phase in the results tuple. - Update test_excitation_phase.py to convert omega to wavenumber correctly and unpack the new tuple return. - Remove redundant and broken test_bem_comparison.py (superseded by test_hydro.py). - Add debug plotting and CSV dumping to test_hydro.py for better traceability.
- Update app.py to use BasicRegionGeometry.from_vectors for robust body construction. - Implement logic to automatically group adjacent segments into bodies based on heaving input (mimicking openflash_utils). - Fix Only 0 or 1 body can be marked as heaving' error by ensuring correct body mapping. - Add input validation to ensure depth, radii, and heaving list lengths match. - Expose excitation phase and force in the single-run results table.
- Create comprehensive Jupyter Notebook for interactive OpenFLASH analysis. - Implement MatrixSpy context manager to intercept and capture the internal system matrix (A) and vector (b) from the MEEMEngine solvers. - Add matrix sparsity visualization matching the style of MEEM.ipynb (blue markers, inverted axis, block boundary grid lines). - Add Numerical vs Analytical comparison logic by exporting/importing the captured matrix to CSV, replicating the legacy verification workflow. - Include standard hydrodynamic visualizations: Potential Field (Real/Imag), Pressure Field, Surface Elevation, and Frequency Sweep (Added Mass, Damping, Excitation Force/Phase).
Updates test_multi_equations.py to match the legacy implementation in multi_equations.py, where R_2n functions (Bessel K and Log) are anchored at the outer radius (a[i]) rather than the inner radius (a[i-1]). - Update test_R_2n_n0 and test_R_2n_n_positive to expect outer-radius anchoring - Update derivative tests (diff_R_2n) to reflect the outer-anchored derivative - Update integral tests (int_R_2n) to use correct normalization denominators and exponential shifts matching the legacy formulation
Cap v openflash
No description provided.