Skip to content

Fix variable re-use in power spectrum#231

Merged
JBorrow merged 2 commits intomasterfrom
fix_variable_reuse
Apr 21, 2025
Merged

Fix variable re-use in power spectrum#231
JBorrow merged 2 commits intomasterfrom
fix_variable_reuse

Conversation

@JBorrow
Copy link
Member

@JBorrow JBorrow commented Apr 21, 2025

Fixes #218

@JBorrow JBorrow requested review from Copilot and kyleaoman April 21, 2025 15:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a variable re-use issue in the power spectrum calculation by renaming intermediate variables during the particle folding process and updates the corresponding docstrings. Key changes include:

  • Introducing new temporary variables (e.g. x_pos_scaled) in deposit to avoid re-using the original coordinate variables.
  • Updating the docstrings for deposit and deposit_parallel to clarify the scaling of the positions.
  • Removing extraneous characters from the parameter descriptions.
Comments suppressed due to low confidence (4)

swiftsimio/visualisation/power_spectrum.py:40

  • The docstring for parameter 'y' incorrectly references 'box_x'. Please update it to 'box_y' to accurately reflect the scaling for y-positions.
array of y-positions of the particles. Scaled by box_x by this function, so ensure they have the same units.

swiftsimio/visualisation/power_spectrum.py:43

  • The docstring for parameter 'z' incorrectly references 'box_x'. Please update it to 'box_z' to accurately reflect the scaling for z-positions.
array of z-positions of the particles. Scaled by box_x by this function, so ensure they have the same units.

swiftsimio/visualisation/power_spectrum.py:123

  • In the deposit_parallel docstring, parameter 'y' should reference 'box_y' instead of 'box_x' for clarity and correctness in unit scaling.
array of y-positions of the particles. Scaled by box_x by this function, so ensure they have the same units.

swiftsimio/visualisation/power_spectrum.py:126

  • In the deposit_parallel docstring, parameter 'z' should reference 'box_z' instead of 'box_x' for clarity and correctness in unit scaling.
array of z-positions of the particles. Scaled by box_x by this function, so ensure they have the same units.

Copy link
Member

Choose a reason for hiding this comment

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

Is "same units" required? Or is "compatible units" enough? cosmo_array should be clever enough now for the latter. Might want to check that mixed physical & comoving inputs give intuitive outputs, do you want to explicitly coerce the box size to match the comoving flag of the coordinates?

Copy link
Member Author

Choose a reason for hiding this comment

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

This function explicitly asks for numpy arrays because of numba acceleration (value arrays; that's also what is passed by calling functions) so all unit coercion should be done before touching it.

@JBorrow JBorrow merged commit 23bf7dc into master Apr 21, 2025
8 checks passed
@JBorrow JBorrow deleted the fix_variable_reuse branch April 21, 2025 21:31
stankortmann pushed a commit to stankortmann/swiftsimio that referenced this pull request Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Computation power spectra with folding

3 participants