Skip to content

Adding a PyCBC plugin to generate ESIGMA waveforms#1

Open
divyajyoti09 wants to merge 1 commit intogwnrtools:masterfrom
divyajyoti09:pycbc_esigma_plugin
Open

Adding a PyCBC plugin to generate ESIGMA waveforms#1
divyajyoti09 wants to merge 1 commit intogwnrtools:masterfrom
divyajyoti09:pycbc_esigma_plugin

Conversation

@divyajyoti09
Copy link
Copy Markdown

This plugin is added to generate ESIGMA waveforms in PyCBC without additional settings or specialized PyCBC installation

@prayush prayush requested a review from Copilot April 13, 2025 19:40
Copy link
Copy Markdown

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 4 out of 4 changed files in this pull request and generated 2 comments.


if not import_os_exists or not lal_data_path_exists:
# Find the index where to insert the LAL_DATA_PATH line
insert_index = None
Copy link

Copilot AI Apr 13, 2025

Choose a reason for hiding this comment

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

If no 'import' statements are found in esigma_utils.py, the insert_index remains None and no insertion is performed. Consider adding a fallback insertion index, such as appending to the file, to guarantee that the LAL_DATA_PATH line is inserted.

Copilot uses AI. Check for mistakes.
Comment on lines +76 to +80
hp_ts = TimeSeries(hp, input_params['delta_t'])
hc_ts = TimeSeries(hc, input_params['delta_t'])

hp_tapered = taper_signal(hp_ts)
hc_tapered = taper_signal(hc_ts)
Copy link

Copilot AI Apr 13, 2025

Choose a reason for hiding this comment

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

[nitpick] The logic for converting the waveform output into TimeSeries and tapering is duplicated across several waveform functions. Consider refactoring the repeated logic into a common helper function to improve maintainability.

Suggested change
hp_ts = TimeSeries(hp, input_params['delta_t'])
hc_ts = TimeSeries(hc, input_params['delta_t'])
hp_tapered = taper_signal(hp_ts)
hc_tapered = taper_signal(hc_ts)
hp_tapered, hc_tapered = process_waveform(hp, hc, input_params['delta_t'])

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@prayush prayush Apr 13, 2025

Choose a reason for hiding this comment

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

@divyajyoti09 This file is not needed, the plugin should be installed when this package esigmapy is installed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@divyajyoti09 This file is probably not needed in its current form, the plugin should be installed when this package esigmapy itself is installed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@divyajyoti09 add the information present here in a suitable way to the README of this project.

Copy link
Copy Markdown
Contributor

@prayush prayush left a comment

Choose a reason for hiding this comment

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

Please see the comments above

@ahnitz ahnitz mentioned this pull request Mar 18, 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.

3 participants