TST: change all likelihood and waveform tests to use more recent waveforms (#791)#897
TST: change all likelihood and waveform tests to use more recent waveforms (#791)#897akjm99 wants to merge 10 commits intobilby-dev:mainfrom
Conversation
GregoryAshton
left a comment
GregoryAshton
left a comment
There was a problem hiding this comment.
Overall looks good thanks @akjm99. Just need to revert the cases where it is using an ROQ.
|
|
||
| trial_roq_paths = [ | ||
| "/roq_basis", | ||
| os.path.join(os.path.expanduser("~"), "ROQ_data/IMRPhenomPv2/4s"), |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
| # 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"), |
There was a problem hiding this comment.
Same as the case above
There was a problem hiding this comment.
And applied to the comments as well
| # 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"), |
| # 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"), |
|
@akjm99 it looks like a commit from |
|
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...) |
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. |
|
@akjm99 do you have time to have another look at this? I think ROQ tests just need switch to IMRPhenomPv2. |
| frequency_nodes_linear=fnodes_linear, | ||
| frequency_nodes_quadratic=fnodes_quadratic, | ||
| reference_frequency=50.0, | ||
| waveform_approximant="IMRPhenomPv2", |
There was a problem hiding this comment.
I think this line should be using IMRPhenomPv2 since we are passing in the ROQ terms just above
3 participants
No description provided.