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?
- Added _ensure_m_k_and_N_k_arrays to MEEMEngine to avoid repetition - Updated assemble_A_multi and assemble_b_multi to use the helper - Improves code reuse and robustness - change function names from _og to _full
…from the full A computation
- Added `from __future__ import annotations` to support forward references. - Annotated return type of run_and_store_results as Results (no string). - Enhanced docstring with clearer parameter and return descriptions. - Added type annotations for intermediate variables and ensured shape checks. - Improved output logging and placeholder potential handling.
…sure frequency consistency - Use problem.frequencies as the single source of truth for all MEEM computations. - Eliminated redundant m0_values parameter to prevent mismatches and confusion. - Simplified loop logic by iterating directly over problem.frequencies. - Updated docstring accordingly for clarity and correctness.
- use problem.frequencies as source of truth for consistency and robustness
…andling - Removed m0 from Domain; use problem.frequencies globally for consistency. - Added eigenfunction placeholder property to Domain. - Enforced shared h, di, a parameters via Geometry when creating Domain objects. - Added Geometry.adjacency_matrix property for domain adjacency logic. - Improved Domain property methods for r, z coordinates and physical params. - Cleaned up MEEMProblem to rely on Geometry and manage frequencies and modes.
…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
rebeccamccabe
temporarily deployed
to
github-pages
change contains == false to !contains
Switches the documentation preview artifact compression from 'tar' to 'zip' in the 'deploy-docs.yml' workflow. This resolves an issue where the artifact could not be easily downloaded and extracted directly through the GitHub Actions UI, which expects a standard ZIP format.
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
No description provided.