Skip to content

Conversation

@lispandfound
Copy link
Contributor

This pull request introduces a new workflow stage merge-lf and refactors the hf-sim and bb-sim stages to use xarray. This is part of the ongoing project to standardise inputs and outputs on accepted industry standard file formats. Leaving as a draft for the time being until I can test it properly.

Copilot AI review requested due to automatic review settings April 17, 2025 03:46
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.

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

workflow/scripts/bb_sim.py:291

  • [nitpick] Consider renaming 'station_file_v30' to 'station_vs30_df' to more clearly indicate that this variable holds a DataFrame.
station_file_v30 = pd.read_csv(

workflow/scripts/hf_sim.py:264

  • [nitpick] Consider rephrasing the comment to clarify that 'dt' is directly obtained from hf_config rather than being calculated from duration and sample count.
dt = hf_config.dt  # Calculate dt from duration and number of samples

@lispandfound lispandfound marked this pull request as draft April 17, 2025 03:53
@lispandfound lispandfound requested a review from Copilot April 17, 2025 03:56
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 adds a new workflow stage for merging low-frequency waveform data into an xarray dataset while refactoring the high-frequency and broadband simulation stages to use xarray. Key changes include:

  • Introduction of the new merge_lf stage in its own script.
  • Refactoring of hf_sim to read and write data using xarray.
  • Major updates in bb_sim to convert inputs from h5py files to xarray datasets and to better align station metadata.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
workflow/scripts/merge_lf.py New script for merging low-frequency EMOD3D data into an xarray dataset.
workflow/scripts/hf_sim.py Refactored HF simulation stage using xarray for dataset creation and file output.
workflow/scripts/bb_sim.py Converted broadband simulation stage to use xarray datasets; adjusted station handling.
tests/test_cli.py Updated CLI tests to include the new merge_lf command.
pyproject.toml Added the merge-lf command entry to the project configuration.

},
)
# Ensure that LF and HF agree on station list
common_stations = list(set(hf_ds.station.values) & set(lf_ds.station.values))
Copy link

Copilot AI Apr 17, 2025

Choose a reason for hiding this comment

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

Using a set intersection to determine common_stations does not preserve a consistent order, which may lead to misalignment between station data and coordinate arrays. Consider sorting the list or using an intersection method that retains order to ensure consistency.

Suggested change
common_stations = list(set(hf_ds.station.values) & set(lf_ds.station.values))
common_stations = [station for station in hf_ds.station.values if station in lf_ds.station.values]

Copilot uses AI. Check for mistakes.
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.

2 participants