Skip to content

Avoid unneeded interpolation in map_to_superrest_frame - #103

Closed
akhairna wants to merge 3 commits into
moble:mainfrom
akhairna:map_to_superrest
Closed

Avoid unneeded interpolation in map_to_superrest_frame#103
akhairna wants to merge 3 commits into
moble:mainfrom
akhairna:map_to_superrest

Conversation

@akhairna

@akhairna akhairna commented Apr 3, 2025

Copy link
Copy Markdown
Contributor

Hi @moble & @duetosymmetry. I have replaced the interpolation with slicing in the map_to_superrest_frame function. Also, I spotted two typos in the description that I changed. This branch is rebased on top of the previous slicing-abd branch. Please review the PR.

@duetosymmetry

Copy link
Copy Markdown
Contributor

This PR is dependent on #101 (@moble, do you have a label?)

@duetosymmetry

Copy link
Copy Markdown
Contributor

@akhairna a better name for this PR would be "Avoid unneeded interpolation in map_to_superrest_frame"

@duetosymmetry

Copy link
Copy Markdown
Contributor

@keefemitman, Aniket found that the parameter fix_time_phase_freedom is just not used in map_to_superrest_frame. Was it supposed to be used for something? Instead of Aniket's documentation fix, we could just remove the parameter...

@keefemitman

Copy link
Copy Markdown
Contributor

Ah no this is suppose to call sxs.waveforms.alignment.align2d, but it looks like I never submitted a PR for this change which I had made locally. I'll submit a PR to do this tomorrow.

@moble

moble commented Apr 4, 2025

Copy link
Copy Markdown
Owner

This looks like it replaces #101, no?

@akhairna

akhairna commented Apr 4, 2025

Copy link
Copy Markdown
Contributor Author

Yes, there would be some conflicts. I will be rebasing my commit on top of @keefemitman 's PR. Then my PR will be ready to review.

@duetosymmetry

Copy link
Copy Markdown
Contributor

#101 is not Keefe's PR, it is Aniket's earlier PR. The first two commits of this PR are the two commits from #101 (Aniket's original commit, plug Mike's merge commit). If #101 is merged, then this PR is atomic, just getting rid of interpolation when slicing could be used. @moble up to you if you want to review/merge them separately, or treat this as replacing #101.

@akhairna akhairna changed the title Map to superrest Avoid unneeded interpolation in map_to_superrest_frame Apr 7, 2025
@akhairna akhairna closed this Apr 12, 2025
@akhairna
akhairna deleted the map_to_superrest branch April 17, 2025 17:12
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.

4 participants