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

Montecarlo configuration refactor, Numba developer proposed version #2568

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

andrewfullard
Copy link
Contributor

@andrewfullard andrewfullard commented Apr 1, 2024

📝 Description

Type: 🪲 bugfix

This is the version of the refactor that uses the methods proposed here: https://hackmd.io/Wmy-e2RsQCSqjJsP0VAmbg?both which allow us to send config objects to different functions with smart compilation that accounts for changes in the config while retaining the performance benefits of skipping paths (this effectively compiles JIT functions to each config).

🚦 Testing

How did you test these changes?

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

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

Attention: Patch coverage is 60.59113% with 80 lines in your changes are missing coverage. Please review.

Project coverage is 67.99%. Comparing base (328ec77) to head (8224fdc).
Report is 2 commits behind head on master.

❗ Current head 8224fdc differs from pull request most recent head 6d06b34. Consider uploading reports for the commit 6d06b34 to get more accurate results

Files Patch % Lines
.../montecarlo/estimators/radfield_estimator_calcs.py 39.28% 17 Missing ⚠️
.../montecarlo/montecarlo_numba/single_packet_loop.py 5.55% 17 Missing ⚠️
tardis/montecarlo/montecarlo_numba/base.py 0.00% 9 Missing ⚠️
tardis/montecarlo/montecarlo_numba/interaction.py 38.46% 8 Missing ⚠️
tardis/montecarlo/montecarlo_numba/vpacket.py 0.00% 8 Missing ⚠️
tardis/transport/r_packet_transport.py 28.57% 5 Missing ⚠️
...is/montecarlo/estimators/radfield_mc_estimators.py 42.85% 4 Missing ⚠️
tardis/montecarlo/montecarlo_configuration.py 95.16% 3 Missing ⚠️
tardis/montecarlo/tests/test_montecarlo.py 0.00% 3 Missing ⚠️
tardis/simulation/base.py 60.00% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2568      +/-   ##
==========================================
+ Coverage   67.18%   67.99%   +0.80%     
==========================================
  Files         173      171       -2     
  Lines       14570    14259     -311     
==========================================
- Hits         9789     9695      -94     
+ Misses       4781     4564     -217     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewfullard
Copy link
Contributor Author

andrewfullard commented Apr 1, 2024

Total test duration (Moria): 683s
Total test duration #2525 (Moria): 622s

These times are quite reliable and reproducible.

This is likely because the obj2strdict is being called in the single packet loop. I believe to get the best benefit of this method would require splitting our physics functions up wherever they have if-statements based on the configuration.

@andrewfullard
Copy link
Contributor Author

need to rebase

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.

None yet

1 participant