Tests for doppler factor functions

[sonachitchyan]

📝 Description

Type: 🪲 bugfix | 🚀 feature | ☣️ breaking change | 🚦 testing | 📝 documentation | 🎢 infrastructure

Add tests for doppler factor full_relativiry and partial_relativity

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

• Testing pipeline
• Other method (describe)
• My changes can't be tested (explain why)

☑️ Checklist

• I requested two reviewers for this pull request
• I updated the documentation according to my changes
• I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

[codecov]

Codecov Report

Merging #2379 (4f219ae) into master (5b36105) will decrease coverage by 1.94%.
Report is 6 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 4f219ae differs from pull request most recent head e6014cf. Consider uploading reports for the commit e6014cf to get more accurate results

@@            Coverage Diff             @@
##           master    #2379      +/-   ##
==========================================
- Coverage   70.57%   68.64%   -1.94%     
==========================================
  Files         139      145       +6     
  Lines       12875    13251     +376     
==========================================
+ Hits         9087     9096       +9     
- Misses       3788     4155     +367     

Files Changed	Coverage Δ
tardis/montecarlo/montecarlo_numba/base.py	32.53% <ø> (ø)
...s/montecarlo/montecarlo_numba/tests/test_packet.py	97.50% <ø> (-0.20%)	⬇️
tardis/montecarlo/tests/test_montecarlo.py	29.38% <ø> (-0.77%)	⬇️
tardis/transport/tests/test_doppler_factor.py	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

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

[chvogl]

The tests look good. However, I think it would be good if all tests relating to frame transformations are in the same file. There are already tests in tardis/montecarlo/tests/test_montecarlo.py. You could move your tests there or vice versa. In principle, the tests could also go into a test file in transport where the frame transformations live. Btw test_get_doppler_factor seems to be duplicated. It is in this file and tardis/montecarlo/tests/test_montecarlo.py. Maybe remove the duplicate?

[sonachitchyan]

I did this, can you take a look again?

[andrewfullard]

Looks good. Could slap in some basic docstrings so all the boxes are checked.

[chvogl]

There still seem to be some duplicated tests (from test_montecarlo.py). Also, I would move all of the doppler factors tests to the new test file.

[chvogl]

There are more doppler factor tests below. I would also move them to the new file.

[sonachitchyan]

@chvogl the tests below fail because of this line. Seems like the SuperluminousError is not defined anywhere?

[chvogl]

I agree. It looks like it doesn't exist anymore. I also just noticed that all tests in test_montecarlo.py are skipped because of pytestmark = pytest.mark.skip(reason="Port from C to numba"). This explains why the tests did not fail.

[chvogl]

Can you make an issue that we should check if there are any tests in test_montecarlo.py that we actually need but are currently skipped?

[chvogl]

Looks good to me! However, we should figure out if we need any of the tests from test_montecarlo that are currently skipped.

[chvogl]

I agree. It looks like it doesn't exist anymore. I also just noticed that all tests in test_montecarlo.py are skipped because of pytestmark = pytest.mark.skip(reason="Port from C to numba"). This explains why the tests did not fail.

[chvogl]

Can you make an issue that we should check if there are any tests in test_montecarlo.py that we actually need but are currently skipped?