Skip to content
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

Merged
merged 11 commits into from
Jun 9, 2021
Merged

Fix for close line test. #1565

merged 11 commits into from
Jun 9, 2021

Conversation

Rodot-
Copy link
Contributor

@Rodot- Rodot- commented May 12, 2021

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?

  • Testing pipeline.
  • Other.
    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

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@andrewfullard
Copy link
Contributor

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build failed 1dc6edf

Click here to see results.

@andrewfullard
Copy link
Contributor

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build failed 6b553e6

Click here to see results.

@andrewfullard
Copy link
Contributor

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Rodot-
Copy link
Contributor Author

Rodot- commented May 20, 2021

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build succeeded 3ef297c

Click here to see results.

@andrewfullard
Copy link
Contributor

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build succeeded 3ef297c

Click here to see results.

@Rodot- Rodot- requested a review from afloers May 24, 2021 14:55
@andrewfullard andrewfullard requested a review from chvogl May 26, 2021 15:51
@Rodot-
Copy link
Contributor Author

Rodot- commented May 26, 2021

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build succeeded c303c43

Click here to see results.

@Rodot-
Copy link
Contributor Author

Rodot- commented Jun 2, 2021

/azp run update-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Update failed 217facd

For more information, check the job log.

@Rodot-
Copy link
Contributor Author

Rodot- commented Jun 2, 2021

/azp run update-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Update succeeded 202d202

Changes pushed to tardis-sn/tardis-refdata.

@Rodot-
Copy link
Contributor Author

Rodot- commented Jun 7, 2021

/azp run compare-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Build succeeded 4fcacc8

Click here to see results.

@Rodot-
Copy link
Contributor Author

Rodot- commented Jun 7, 2021

@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?

@epassaro
Copy link
Member

epassaro commented Jun 8, 2021

  1. The reference data has been updated correctly according to the latest comparison (the refdata from the repo is the same as the one generated by the tip of this PR).
  2. Tests are using the updated reference data because started failing in the master branch (as other PRs as well).
  3. The latest fix for the TARDIS env passed on Sunday https://dev.azure.com/tardis-sn/TARDIS/_build/results?buildId=4220&view=results
  4. There's a slight chance that some package updated recently and broke the tests and it's not your fault. Save your env by doing conda env export > env_jack.yml and post it on Slack.

EDIT: 5. Also I've noticed in your PR just one test is failing: tardis/montecarlo/montecarlo_numba/tests/test_base.py , while in master a lot (formal integral). So the pipeline it's using the correct reference data!.

@epassaro
Copy link
Member

epassaro commented Jun 8, 2021

  1. The reference data has been updated correctly according to the latest comparison (the refdata from the repo is the same as the one generated by the tip of this PR).
  2. Tests are using the updated reference data because started failing in the master branch (as other PRs as well).
  3. The latest fix for the TARDIS env passed on Sunday https://dev.azure.com/tardis-sn/TARDIS/_build/results?buildId=4220&view=results
  4. There's a slight chance that some package updated recently and broke the tests and it's not your fault. Save your env by doing conda env export > env_jack.yml and post it on Slack.

EDIT: 5. Also I've noticed in your PR just one test is failing: tardis/montecarlo/montecarlo_numba/tests/test_base.py , while in master a lot (formal integral). So the pipeline it's using the correct reference data!.

Ok, as you discovered the problem is the new montecarlo_1e5_compare_data.h5 is not pushed to tardis-refdata.

@epassaro epassaro mentioned this pull request Jun 8, 2021
10 tasks
@andrewfullard andrewfullard marked this pull request as ready for review June 8, 2021 15:46
@Rodot-
Copy link
Contributor Author

Rodot- commented Jun 8, 2021

/azp run update-refdata

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-pipelines
Copy link

Update succeeded 39c12e3

Changes pushed to tardis-sn/tardis-refdata.

Rodot- added 9 commits June 8, 2021 12:27
…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
…compare output estimators, energies, and frequencies. This test should now compare a PR to the last stable tardis version
@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #1565 (f374ada) into master (e062c69) will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
...dis/montecarlo/montecarlo_numba/tests/test_base.py 97.14% <0.00%> (-2.86%) ⬇️
...rdis/montecarlo/montecarlo_numba/tests/conftest.py 91.11% <0.00%> (ø)
...s/montecarlo/montecarlo_numba/tests/test_packet.py 100.00% <0.00%> (ø)
.../montecarlo/montecarlo_numba/tests/test_vpacket.py 100.00% <0.00%> (ø)
...dis/tardis/montecarlo/montecarlo_numba/r_packet.py 25.43% <0.00%> (+0.22%) ⬆️
tardis/tardis/montecarlo/montecarlo_numba/base.py 30.00% <0.00%> (+0.37%) ⬆️
...montecarlo/montecarlo_numba/calculate_distances.py 23.68% <0.00%> (+0.60%) ⬆️
...rdis/tardis/montecarlo/montecarlo_numba/vpacket.py 18.36% <0.00%> (+0.89%) ⬆️
.../tardis/montecarlo/montecarlo_numba/interaction.py 29.72% <0.00%> (+1.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 802fab9...f374ada. Read the comment docs.

Copy link
Member

@wkerzendorf wkerzendorf left a 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.
@andrewfullard andrewfullard merged commit 576038a into master Jun 9, 2021
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants