Faster IM Calc for Obs data #181
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
IM/ko_matrices.py:29
- [nitpick] The filename format here (using size - 1) may not match the naming convention used in the KO matrix generator script; consider verifying the consistency between file creation and retrieval.
ko_matrix_file = directory / f"KO_{size - 1}.npy"
IM/ims.py:125
- [nitpick] Consider annotating the 'out' parameter as Optional[np.ndarray] for clarity, ensuring its intended nullable nature is explicitly documented.
def rotate_components(step_000: np.ndarray, step_090: np.ndarray, theta: np.ndarray, out: np.ndarray =None, use_numexpr: bool =True):
IM/im_calculation.py:189
- [nitpick] The raised error message for a missing KO directory could be reworded to more clearly indicate that a valid path must be provided when FAS calculations are required.
if ko_directory is None and IM.FAS in ims_list:
There was a problem hiding this comment.
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
IM/ko_matrices.py:29
- Ensure that using 'size - 1' in the filename is intentional and consistent with how the KO matrices are generated (in gen_ko_matrix.py, the filename is based on the generated 'n' value).
ko_matrix_file = directory / f"KO_{size - 1}.npy"
IM/im_calculation.py:141
- The default value for 'use_numexpr' is False here, while other functions and tests often use True. Consider unifying the default values to avoid unexpected performance differences.
use_numexpr: bool = False,
lispandfound
requested changes
Apr 7, 2025
As a note, dt does not matter for us when generating the KO matrix using numpy rfft |
lispandfound
requested changes
Apr 8, 2025
Co-authored-by: Jake Faulkner <jakefaulkn@gmail.com>
There was a problem hiding this comment.
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
IM/ko_matrices.py:29
- Ensure that the file naming offset (using size - 1) matches the generated filename pattern from gen_ko_matrix.py. If the intention is to use the matrix dimension 'n' directly, adjust the file naming accordingly.
ko_matrix_file = directory / f"KO_{size - 1}.npy"
IM/im_calculation.py:140
- [nitpick] There is an inconsistency in the default value for use_numexpr between calculate_ims (defaulting to False) and the lower-level functions (defaulting to True). Verify that this discrepancy is intentional and aligns with the desired performance tuning.
ko_directory: Path = None,
lispandfound
approved these changes
Apr 10, 2025
AndrewRidden-Harper
approved these changes
Apr 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
2E96
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Speed up improvements by allowing the use of no numexpr.
Deals with pykooh multiprocessing performance issues by switching back to old method of producing KO matrix files.
Improve FAS calculation speed under different scenarios of cores / number of stations to compute for.
Test on all IM's for a single waveform
Before time taken: 21.5s
After time taken: 6.79 s