-
Notifications
You must be signed in to change notification settings - Fork 26
Conversation
02d9b15
to
16ec94a
Compare
Pull Request Test Coverage Report for Build 70
💛 - Coveralls |
f8b62b0
to
4f15b2c
Compare
- renamed test_gwin -> test_sampler - added test_likelihood (incomplete) - added utils module with lots of fixtures - added conftest.py with `--approximants` CLO
does not include any tests that require reading files (yet)
so that we don't need multiple try/except imports across the test modules
a599a5a
to
790ccfd
Compare
DEFAULT_APPROXIMANTS = [ | ||
'TaylorF2', | ||
'IMRPhenomPv2', | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add SEOBNRv4 to the default list? How long do the test take as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vivienr, on my macbook I get the following error:
ValueError: Approximant SEOBNRv4 not available
Is there something special that needs to be done to set up this approximant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a consequence of the test only being set up for frequency-domain waveforms, at the moment. Hopefully I can work with @cdcapano in a future PR to add support for TD waveform approximants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only available in the time domain, and the tests only do calls in the frequency domain. If that's too much troubles, let's look at implementing time domain versions in another patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duncanmmacleod I feel like some of the codeclimate thresholds are a bit low. Like this is considered a failure:
Function test_add_sampler_option_group has 26 lines of code (exceeds 25 allowed). Consider refactoring.
Maybe at some future point these could be relaxed a bit.
Otherwise, this PR looks fine.
@cmbiwer: I agree. In particular, I'd up the lines of code allowed to 50, the cognitive complexity to 10, allow up to 8 arguments to functions, and 750 lines of codes per file. |
This PR updates the test suite to include a second test case module! and lots of other things:
test/test_gwin.py
->test/test_sampler.py
test/test_io.py
(incomplete)test/test_likelihood.py
(incomplete)test/test_option_utils.py
(incomplete)test/test_results.py
(incomplete)test/utils
module with lots of fixturesconftest.py
with--approximants
command-line optionsThe final point means that the suite can now be run using the default approximants with
but you can specify which approximants to use:
Closes #11.
Fixes #16.