Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: change all likelihood and waveform tests to use more recent waveforms (#791) #897

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

akjm99
Copy link
Collaborator

@akjm99 akjm99 commented Jan 29, 2025

No description provided.

@akjm99 akjm99 linked an issue Jan 29, 2025 that may be closed by this pull request
@akjm99 akjm99 requested a review from GregoryAshton January 29, 2025 16:28
Copy link
Collaborator

@GregoryAshton GregoryAshton left a comment

Choose a reason for hiding this comment

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

Overall looks good thanks @akjm99. Just need to revert the cases where it is using an ROQ.

@@ -231,8 +231,8 @@ def setUp(self):

trial_roq_paths = [
"/roq_basis",
os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, I don't think we have ROQ data for XPHM yet, so this will need to be reverted (and may be why the CI is failing).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mj-will I assume #897 was a "move towards" modern waveforms and it is okay to use the older waveforms for the ROQs to avoid having to figure out how to package them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I think that's fine.

I'll note there is definitely ROQ data for XPHM on CIT but I don't think it's public (and is also huge).

@@ -283,8 +283,8 @@ def setUp(self):
# Possible locations for the ROQ: in the docker image, local, or on CIT
trial_roq_paths = [
"/roq_basis",
os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as the case above

Copy link
Collaborator

Choose a reason for hiding this comment

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

And applied to the comments as well

@@ -546,8 +546,8 @@ def test_rescaling(self):
# Possible locations for the ROQ: in the docker image, local, or on CIT
trial_roq_paths = [
"/roq_basis",
os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

And again.

@@ -1132,8 +1132,8 @@ def create_likelihood_single_basis(self):
# Possible locations for the ROQ: in the docker image, local, or on CIT
trial_roq_paths = [
"/roq_basis",
os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here

@mj-will
Copy link
Collaborator

mj-will commented Feb 4, 2025

@akjm99 it looks like a commit from main has made it into the branch and is showing in the diff. I don't think it's from a merge or rebase so I'm slightly unsure what's happened to the history. @GregoryAshton any ideas?

@akjm99 akjm99 requested a review from GregoryAshton February 4, 2025 13:41
@akjm99
Copy link
Collaborator Author

akjm99 commented Feb 4, 2025

Hi @mj-will. I am not sure how that happened sorry! I followed the contributing guidelines which said to pull upstream, make edits, and then push to fork (?). When I tried to push my changes to my fork, it said there were conflicts and that I had to merge my new changes and merge/rebase my branch to my main (?) (which I think I managed to do, at least I was able to push to my fork after that...)

@mj-will mj-will added the testing label Feb 4, 2025
@mj-will
Copy link
Collaborator

mj-will commented Feb 4, 2025

Hi @mj-will. I am not sure how that happened... I followed the contributing guidelines which said to pull upstream. When I tried to push my changes to my fork, it then said there were conflicts and that I had to merge my new changes and merge/rebase my branch to main (?) (which I think I managed to do, at least I was able to push to my fork after that...)

Hmm, I think the upstream changed between you initially making the branch and now (the commit is from another recent PR). I think you've followed the contributing guidelines correctly but we're missing a section on updating a PR with changes from main after it has been made. I've opened an issue to track this: #901.

Given we'll squash, this probably doesn't matter in this case, and it's probably more work to fix than it's worth.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change all likelihood and waveform tests to use more recent waveforms
3 participants