Skip to content

Bugfix and enhancement in heterodyned likelihood#222

Closed
thomasckng wants to merge 39 commits into
jim-devfrom
fix-maxL
Closed

Bugfix and enhancement in heterodyned likelihood#222
thomasckng wants to merge 39 commits into
jim-devfrom
fix-maxL

Conversation

@thomasckng
Copy link
Copy Markdown
Collaborator

@thomasckng thomasckng commented May 23, 2025

No description provided.

@thomasckng thomasckng marked this pull request as ready for review May 23, 2025 05:49
@thomasckng
Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 23, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@thomasckng thomasckng self-assigned this May 23, 2025
@thomasckng thomasckng added the enhancement New feature or request label May 23, 2025
Comment thread src/jimgw/core/single_event/likelihood.py Outdated
Copy link
Copy Markdown
Collaborator

@SSL32081 SSL32081 left a comment

Choose a reason for hiding this comment

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

This PR removes the evaluate_original function from Heterodyne likelihood, and calls the evaluate function from super class instead.

@SSL32081
Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 26, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@SSL32081 SSL32081 changed the title Remove redundant function in likelihood.py Reuse parent function and minor bugfix in likelihood.py May 26, 2025
Comment thread src/jimgw/core/single_event/likelihood.py Outdated
@kazewong
Copy link
Copy Markdown
Owner

@thomasckng I think this is addressed in #237, can you double check if there is anything remained in this PR that is not included? Otherwise we can close this PR

@thomasckng thomasckng changed the title Merge repeated functions and bugfix in heterodyned likelihood Bugfix and minor enhancement in heterodyned likelihood Jul 22, 2025
@thomasckng
Copy link
Copy Markdown
Collaborator Author

@SSL32081 I have resolved the conflicts. Can you take a look at the recent commits and see if I missed anything?

Comment thread src/jimgw/core/single_event/likelihood.py
@thomasckng thomasckng removed their assignment Jul 22, 2025
@thomasckng thomasckng changed the title Bugfix and minor enhancement in heterodyned likelihood Bugfix and enhancement in heterodyned likelihood Jul 25, 2025
@thomasckng thomasckng self-assigned this Jul 25, 2025
@thomasckng
Copy link
Copy Markdown
Collaborator Author

@SSL32081 I have solved the slow __init__() issue. Can you take a look at 859d97f?

@thomasckng thomasckng requested a review from Copilot July 25, 2025 06:46
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.

Pull Request Overview

This PR addresses bugfixes and enhancements in the heterodyned likelihood implementation. The changes focus on improving code correctness, performance optimization through vectorization, and code maintainability.

Key changes include:

  • Fixed critical bugs in reference parameter handling and frequency variable usage
  • Replaced inefficient loop-based coefficient computation with vectorized operations using JAX/NumPy broadcasting
  • Improved code organization by moving frequency grid initialization to proper location
Comments suppressed due to low confidence (2)

src/jimgw/core/single_event/likelihood.py:492

  • [nitpick] The variable name pol is not descriptive. Consider using polarization or pol_key to make the code more readable and self-documenting.
            jnp.array([jnp.abs(h_sky[pol]) for pol in h_sky.keys()]), axis=0

src/jimgw/core/single_event/likelihood.py:745

  • [nitpick] Using _ to discard the returned rng_key is appropriate, but consider adding a comment explaining what value is being discarded for clarity.
        _, best_fit, log_prob = optimizer.optimize(

Comment thread src/jimgw/core/single_event/likelihood.py
Comment thread src/jimgw/core/single_event/likelihood.py Outdated
Comment thread src/jimgw/core/single_event/likelihood.py
@thomasckng thomasckng requested a review from kazewong July 26, 2025 12:59
@thomasckng
Copy link
Copy Markdown
Collaborator Author

@kazewong This PR has been updated. The major changes are a bugfix at line 558 and a significant enhancement in compute_coefficients().

Repository owner deleted a comment from coderabbitai Bot Jul 26, 2025
@thomasckng thomasckng requested a review from tsunhopang July 26, 2025 13:11
@thomasckng thomasckng removed their assignment Oct 30, 2025
@thomasckng thomasckng closed this Oct 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants