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

Numba montecarlo #929

Merged
merged 119 commits into from
Nov 17, 2020
Merged

Numba montecarlo #929

merged 119 commits into from
Nov 17, 2020

Conversation

wkerzendorf
Copy link
Member

@wkerzendorf wkerzendorf commented May 4, 2019

Things to check if they are done

Things to do

  • line scattering

  • electron scattering

  • virtual packets

  • macroatom

  • packet logging (last_packet_interaction)

Line estimators are slowing the process down - maybe a useful optimization strategy

Final Testing

There are 4 code versions. There is upstream/master there is numba_montecarlo both of which have different seeding strategies and will never be numerically equal (those we will summarize as main). There are two versions that have additional code that will allow logging - those we will call c_compare and numba_compare (and will call the compare branch).

  • virtual - only compare - sica

  • virtual - only compare - ddc10

  • integrated - main only - sica

  • integrated - main only - ddc10

  • estimators - compare only -sica

  • estimators - compare only -ddc10

  • iterations - (compare/main) -sica

  • iterations - (compare/main) -ddc10

  • Need to fix negative boundary distance issue for vpackets: specific case occurs when a packet reaches a boundary with a close line so distance_trace_line = 0 and distance_boundary can be negative (shell ID does not advance)
    The models that shall be run is low velocity Si/Ca and DDC10.

The last step is to get numba to produce a new refdata and look at this in the notebook.

If all these models run then we agree that numba montecarlo can be merged.

Merging Strategy

  1. talk to TARDIS core
  2. if all tests pass - update the version number to 4.0 with the merged numba montecarlo
  3. merge

Post-merge

Start writing unit tests for the numba version
Improve speed.

@wkerzendorf
Copy link
Member Author

There is a sensible discussion about setting r when doing a move_packet_across_shell_boundary - do we move like we normally do - or just set r to the shell boundary??

@wkerzendorf wkerzendorf force-pushed the numba_montecarlo branch 4 times, most recently from 4854f6c to f3494c2 Compare June 12, 2019 16:50
@wkerzendorf wkerzendorf force-pushed the numba_montecarlo branch 3 times, most recently from 9923c54 to ace4eda Compare January 25, 2020 23:37
@@ -0,0 +1,7 @@
from llvmlite import binding
binding.set_option("tmp", "-non-global-value-max-name-size=2048")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering what this is for? IIRC this patch against LLVM "fixed", well, prevented, it from being an issue https://github.com/numba/llvmlite/blob/master/conda-recipes/0001-Revert-Limit-size-of-non-GlobalValue-name.patch .

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to know. Thanks - haven't worked on it in a while.

wkerzendorf and others added 17 commits March 17, 2020 23:07
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Yssavo Camacho-Neves <yic6@physics.rutgers.edu>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
@andrewfullard andrewfullard marked this pull request as ready for review November 3, 2020 20:28
Values are stored in the vpacket collection for each vpacket. Vpacket collections are dumped into a huge list right now.
Need to set the configuration value to True in montecarlo_configuration.py

Needs to be an install option again
Can now be activated as a configuration settings rather than compile-time. Output should now be a proper array of arrays rather than a list of arrays.
tardis/io/schemas/montecarlo.yml Outdated Show resolved Hide resolved
tardis/montecarlo/__init__.py Outdated Show resolved Hide resolved
@@ -0,0 +1,7 @@
from llvmlite import binding
binding.set_option("tmp", "-non-global-value-max-name-size=2048")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
binding.set_option("tmp", "-non-global-value-max-name-size=2048")

tardis/montecarlo/montecarlo_numba/base.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/base.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/tests/test_vpacket.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/vpacket.py Outdated Show resolved Hide resolved
tardis/montecarlo/montecarlo_numba/vpacket.py Outdated Show resolved Hide resolved
tardis/scripts/debug/run_numba_single.run.xml Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 14, 2020

Codecov Report

Merging #929 (d14025e) into master (071a48e) will decrease coverage by 10.32%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #929       +/-   ##
===========================================
- Coverage   81.57%   71.24%   -10.33%     
===========================================
  Files          47       66       +19     
  Lines        3804     5077     +1273     
===========================================
+ Hits         3103     3617      +514     
- Misses        701     1460      +759     
Impacted Files Coverage Δ
tardis/tardis/montecarlo/formal_integral.py 15.78% <0.00%> (-78.81%) ⬇️
tardis/tardis/widgets/line_info.py 75.17% <0.00%> (-20.69%) ⬇️
tardis/tardis/widgets/tests/test_line_info.py 80.85% <0.00%> (-19.15%) ⬇️
tardis/tardis/montecarlo/base.py 84.61% <0.00%> (-2.89%) ⬇️
tardis/tardis/widgets/util.py 82.92% <0.00%> (-2.44%) ⬇️
tardis/tardis/io/config_reader.py 86.02% <0.00%> (ø)
tardis/tardis/montecarlo/struct.py 100.00% <0.00%> (ø)
tardis/tardis/widgets/shell_info.py 96.00% <0.00%> (ø)
tardis/tardis/io/config_validator.py 89.47% <0.00%> (ø)
tardis/tardis/montecarlo/spectrum.py 70.00% <0.00%> (ø)
... and 24 more

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 071a48e...d14025e. Read the comment docs.

Mostly to force a new test run
@wkerzendorf wkerzendorf merged commit 229b676 into master Nov 17, 2020
wkerzendorf added a commit that referenced this pull request Nov 17, 2020
This is a year worth of work from many people and hopefully will make
TARDIS easier to use and develop on.

Co-authored-by: Andrew Fullard <andrewgfullard@gmail.com>
Co-authored-by: Jack O'Brien <jobrien585@gmail.com>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Andreas Flörs <afloers@mpa-garching.mpg.de>
Co-authored-by: Luke Shingles <luke.shingles@gmail.com>
Co-authored-by: Alice Harpole <harpolea@gmail.com>
Co-authored-by: Marc Williamson <marxwillia@gmail.com>
Co-authored-by: Yssa Camacho <yssavo.camacho@gmail.com>
Co-authored-by: Arjun Savel <asavel@berkeley.edu>
Co-authored-by: Stuart Archibald <stuart.archibald@googlemail.com>
Co-authored-by: Siu Kwan Lam <michael.lam.sk@gmail.com>
wkerzendorf added a commit that referenced this pull request Nov 17, 2020
This is a year worth of work from many people and hopefully will make
TARDIS easier to use and develop on.

Co-authored-by: Andrew Fullard <andrewgfullard@gmail.com>
Co-authored-by: Jack O'Brien <jobrien585@gmail.com>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Andreas Flörs <afloers@mpa-garching.mpg.de>
Co-authored-by: Luke Shingles <luke.shingles@gmail.com>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Marc Williamson <marxwillia@gmail.com>
Co-authored-by: Yssa Camacho <yssavo.camacho@gmail.com>
Co-authored-by: Arjun Savel <asavel@berkeley.edu>
Co-authored-by: Stuart Archibald <stuart.archibald@googlemail.com>
Co-authored-by: Siu Kwan Lam <michael.lam.sk@gmail.com>
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
This is a year worth of work from many people and hopefully will make
TARDIS easier to use and develop on.

Co-authored-by: Andrew Fullard <andrewgfullard@gmail.com>
Co-authored-by: Jack O'Brien <jobrien585@gmail.com>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Andreas Flörs <afloers@mpa-garching.mpg.de>
Co-authored-by: Luke Shingles <luke.shingles@gmail.com>
Co-authored-by: Alice Harpole <aliceharpole@gmail.com>
Co-authored-by: Marc Williamson <marxwillia@gmail.com>
Co-authored-by: Yssa Camacho <yssavo.camacho@gmail.com>
Co-authored-by: Arjun Savel <asavel@berkeley.edu>
Co-authored-by: Stuart Archibald <stuart.archibald@googlemail.com>
Co-authored-by: Siu Kwan Lam <michael.lam.sk@gmail.com>
@andrewfullard andrewfullard deleted the numba_montecarlo branch July 20, 2022 15:58
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.

9 participants