gwastro / sbank

GNU General Public License v2.0
4 stars 14 forks source link

Add support for SEOBNRv5 and IMRPhenomXAS waveforms #54

Closed titodalcanton closed 1 year ago

titodalcanton commented 1 year ago

This adds support for the SEOBNRv5, SEOBNRv5_ROM and IMRPhenomXAS waveform models. Note that I also fixed what I think was a typo: SEOBNRv4 was being associated with the SEOBNRv4ROMTemplate class, while I assume it should be associated with SEOBNRv4Template instead. The two classes appear to be functionally equivalent, so this was probably harmless. Please correct me if this is not right.

I have not tested this.

codecov-commenter commented 1 year ago

Codecov Report

Merging #54 (b25c6f9) into master (1fad226) will increase coverage by 0.12%. The diff coverage is 66.67%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
+ Coverage   55.24%   55.36%   +0.12%     
==========================================
  Files          10       10              
  Lines        1155     1167      +12     
==========================================
+ Hits          638      646       +8     
- Misses        517      521       +4     
Flag Coverage Δ
Linux 55.36% <66.67%> (+0.12%) :arrow_up:
macOS 55.36% <66.67%> (+0.12%) :arrow_up:
python3.10 55.36% <66.67%> (+0.12%) :arrow_up:
python3.11 55.36% <66.67%> (+0.12%) :arrow_up:
python3.8 55.41% <66.67%> (+0.12%) :arrow_up:
python3.9 55.41% <66.67%> (+0.12%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
sbank/tau0tau3.py 63.37% <ø> (ø)
sbank/waveforms.py 51.80% <66.67%> (+0.37%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

spxiwh commented 1 year ago

Regarding the test failures, I think the lint things are nothing to do with this, but do need fixing. Do you want me to suggest a patch on to this one to resolve that.

Slightly more concerned about the python3.7 failures. Seems like conda is not able to resolve an environment for python3.7.

titodalcanton commented 1 year ago

I am willing to create a separate PR to improve the Lint failures. Python 3.7 reached EoL (https://endoflife.date/python) so I propose dropping it from the tests. I can open a PR to that end as well.

titodalcanton commented 1 year ago

I ran the existing test with both IMRPhenomXAS and SEOBNRv5_ROM and things seem fine.

Maybe we should have some extra tests with different approximants @spxiwh?