Faster interface VACF calculation #8
Conversation
WalkthroughThis pull request introduces a new class, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant I as InterfaceVACF
participant P as _prepare
participant S as _single_frame
participant PS as calc_power_spectrum
U->>I: Call calc_vacf(parameters)
I->>P: Prepare data for calculation
loop For each frame
I->>S: Process frame data
end
I-->>U: Return computed VACF
U->>PS: Call calc_power_spectrum(vacf, ts, full)
PS-->>U: Return power spectrum (as wave numbers)
sequenceDiagram
participant U as User
participant C as calc_vector_autocorrelation
participant V as Vectors
participant M as Mask
U->>C: Call calc_vector_autocorrelation(max_tau, delta_tau, step, vectors, mask)
C->>C: Generate lag times array (tau)
C->>V: Use vectors for dot product calculations
C->>M: Apply mask for selection
C-->>U: Return tau and normalized ACF values
Poem
✨ Finishing Touches
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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
WatAnalysis/spectrum.py (3)
44-76: Refactor suggestion for a more conciseif fullcheck.
Lines 66-69 can be condensed into a ternary expression per the static analysis hint. This is optional but improves readability.Proposed change:
- if full: - full_vacf = vacf - else: - full_vacf = np.concatenate([vacf[::-1][:-1], vacf]) + full_vacf = vacf if full else np.concatenate([vacf[::-1][:-1], vacf])🧰 Tools
🪛 Ruff (0.8.2)
66-69: Use ternary operator
full_vacf = vacf if full else np.concatenate([vacf[::-1][:-1], vacf])instead ofif-else-blockReplace
if-else-block withfull_vacf = vacf if full else np.concatenate([vacf[::-1][:-1], vacf])(SIM108)
79-193: Deprecation handling suggestion.
This class is markedInterfaceVACFDeprecatedbut doesn’t raise any deprecation warnings. Adding aDeprecationWarningcan help guide users away from the old interface.+import warnings class InterfaceVACFDeprecated(MultiTrajsAnalysisBase): def __init__(...): + warnings.warn( + "InterfaceVACFDeprecated is deprecated and may be removed in a future release.", + category=DeprecationWarning, + stacklevel=2, + ) ...
278-283: Large array allocations in_prepare.
Notice_oxygen_maskand_hydrogen_velocitiescan consume significant memory for large trajectories. If memory usage is a concern, consider lazy loading or partial buffering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
WatAnalysis/spectrum.py(3 hunks)WatAnalysis/waterdynamics.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
WatAnalysis/spectrum.py
66-69: Use ternary operator full_vacf = vacf if full else np.concatenate([vacf[::-1][:-1], vacf]) instead of if-else-block
Replace if-else-block with full_vacf = vacf if full else np.concatenate([vacf[::-1][:-1], vacf])
(SIM108)
🔇 Additional comments (10)
WatAnalysis/waterdynamics.py (3)
2-5: Nice module-level docstring addition.
The descriptive docstring clarifies the purpose of the module and enhances readability.
7-7: Import statement looks good.
Usingnumpyis essential for vectorized operations and numerical computations in subsequent functions.
271-331: Potential division-by-zero in ACF normalization.
If no vectors are selected or if the zero-lag dot product is 0, thenacf[0]will be zero, causing a division-by-zero issue in line 329 (acf /= acf[0]). It's safer to check before normalizing or ensure data availability.Example adjustment:
... # Normalize the ACF - acf /= acf[0] # Normalize by the zero-lag value + if acf[0] != 0: + acf /= acf[0] + else: + # Handle the zero-lag == 0 scenario, e.g., set ACF to zeros or skip normalization + acf[:] = 0 return tau, acfWatAnalysis/spectrum.py (7)
2-5: Good module-level docstring update.
It succinctly describes the scope of vibrational spectra computations.
10-10: Import of speed_of_light is appropriate.
Used below for frequency-to-wavenumber conversions. No issues found.
16-16: New import from waterdynamics looks correct.
This import referencescalc_vector_autocorrelation, aligning with the new VACF logic.
195-234: Comprehensive class docstring.
The explanation ofInterfaceVACFparameters and usage is clear, aiding maintainability.
236-283: Validatesurf_idsand interval usage in the constructor.
Ifsurf_idsisNoneor empty, surface-related logic may break. Consider adding checks to ensure they are valid or raise an exception early.
285-329: Index-based surface approach.
Method_single_framedepends onself.surf_idsto computesurf1_zandsurf2_z. Ifsurf_idsis out of range or misspecified, it will raise an indexing error. A prior check in the constructor or_prepareensures robust error handling.
330-376: Ensure hydrogen selection is non-empty incalc_vacf.
If no hydrogens are selected for a given frame,mask_ts[sel_hydrogen_ids]might be empty or cause unexpected results in the autocorrelation. Consider a fallback or user notification ifsel_hydrogen_idsis empty.
|
Merge conflict probably requires merging master into devel first |
Hi, I've added a new class that first saves all the data in memory and computes the VACF only at the end.
Instead of computing it in
_concludeI made a separate method, so that users can play around with the settings in a notebook more easily. Thecalc_vacffunction includes acorrelation_stepparameter similar in MDAnalysis' waterdynamics code. It speeds up calculations, convenient for notebook use.I also changed
calc_power_spectrumto directly convert to1/cm, given a timestep in ps, as this is the common unit.The VACF results are the same (given the same data slice and ACF parameters):
You can remove the deprecated class or keep it. I'll also use the
calc_vector_autocorrelationfunction for water dipole calculations (next PR).I can add tests in the future.