-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fix for close line test. #1565
Fix for close line test. #1565
Conversation
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Build failed 1dc6edf |
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Build failed 6b553e6 |
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Build succeeded 3ef297c |
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Build succeeded 3ef297c |
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Build succeeded c303c43 |
/azp run update-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run update-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Update succeeded 202d202 |
/azp run compare-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Build succeeded 4fcacc8 |
@andrewfullard @epassaro it seems that running the compare-refdata after updating the refdata works, but the tests appear to be still using the old ref-data. Am I correct on this or am I missing something? |
EDIT: 5. Also I've noticed in your PR just one test is failing: |
Ok, as you discovered the problem is the new |
/azp run update-refdata |
Azure Pipelines successfully started running 1 pipeline(s). |
Update succeeded 39c12e3 |
…for close lines as much as check for numerical errors when computing the comving fredquency. The test now occurs in calculate_distance_line. The packet specs have been updated to remove the close line flags and the tests for the close lines have been removed
…s when generating references
…compare output estimators, energies, and frequencies. This test should now compare a PR to the last stable tardis version
Codecov Report
@@ Coverage Diff @@
## master #1565 +/- ##
==========================================
- Coverage 67.20% 67.13% -0.08%
==========================================
Files 73 73
Lines 6150 6100 -50
==========================================
- Hits 4133 4095 -38
+ Misses 2017 2005 -12
Continue to review full report at Codecov.
|
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.
looks good
…a full test and isn't directly testing mainloop on its own.
* Fix for close line test. Appears that we don't actually want to look for close lines as much as check for numerical errors when computing the comving fredquency. The test now occurs in calculate_distance_line. The packet specs have been updated to remove the close line flags and the tests for the close lines have been removed * Removed close_line parameter initialization for r_packet * prevented test_montecarlo_main_loop from running comparison to C_tests when generating references * Updated the value of the CLOSE_LINE_THRESHOLD to the double precision limit * Removed statement claiming we are running C close lines (cause we are not anymore) * Empty commit to rerun tests * Updated montecarlo numba tests to do a full test of a tardis run and compare output estimators, energies, and frequencies. This test should now compare a PR to the last stable tardis version * useless commit to get the tests running again * another empty commit for tests * updated test so that it no longer import unecessary modules * Cleaned up base test a tiny bit, really should be noted that this is a full test and isn't directly testing mainloop on its own.
Appears that we don't actually want to look for close lines as much as check for numerical errors when computing the comoving frequency. The test now occurs in calculate_distance_line. The packet specs have been updated to remove the close line flags and the tests for the close lines have been removed
Removed all close line tests. calculate_distance_line now checks if the nu_diff is within CLOSE_LINE_THRESHOLD (Which is a configurable parameter, a better value should be chosen, anyone have any ideas?)
Motivation and context
The close line checks were producing incorrect results before.
How has this been tested?
Since these changes change the way the spectrum is computed, the ref-data needs to be regenerated after this is merged. For the time being, testing has been done by tracking packets manually and running the quickstart notebook.
Type of change
Checklist