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

Made new classes for updated geometry #2085

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

Conversation

nolanbrown01
Copy link
Contributor

Co-authored-by: Sona Chitchyan [email protected]
Co-authored-by: AlexHls [email protected]
Co-authored-by: Wolfgang Kerzendorf [email protected]
Co-authored-by: Joshua Shields [email protected]

📝 Description

Type: 🎢 infrastructure

In numba_interface.py, wrote new classes GeometryGridSpherical1D, GeometryGridCartesian2D, and GeometryGridCartesian3D and appropriate numba_spec files. These will be used by TARDIS as different geometries for different explosions. We also made
calculate_distance_new.py, and copied the 1D spherical distance calculation into it. We will put all future distance calculations in it. Made a fix to a conditional in gamma_ray_grid.py

🚦 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.

Co-authored-by: AlexHls <[email protected]>
Co-authored-by: Wolfgang Kerzendorf <[email protected]>
Co-authored-by: Joshua Shields <[email protected]>
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Co-authored-by: AlexHls <[email protected]>
Co-authored-by: Joshua Shields <[email protected]>
@nolanbrown01
Copy link
Contributor Author

Removed calculate_distance_new from PR

@nolanbrown01 nolanbrown01 marked this pull request as ready for review July 7, 2022 20:29

if shortest == (inner_1 or inner_2):
if (shortest == inner_1) or (shortest == inner_2):
Copy link
Member

Choose a reason for hiding this comment

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

remove from PR.

from tardis.energy_input.util import H_CGS_KEV


@pytest.fixture(scope="function")
Copy link
Member

Choose a reason for hiding this comment

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

remove as not relevant to PR.

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #2085 (8d586d0) into master (a57ab47) will increase coverage by 0.08%.
The diff coverage is 22.03%.

❗ Current head 8d586d0 differs from pull request most recent head 69eca87. Consider uploading reports for the commit 69eca87 to get more accurate results

@@            Coverage Diff             @@
##           master    #2085      +/-   ##
==========================================
+ Coverage   58.04%   58.13%   +0.08%     
==========================================
  Files          76       75       -1     
  Lines        8743     8623     -120     
==========================================
- Hits         5075     5013      -62     
+ Misses       3668     3610      -58     
Impacted Files Coverage Δ
tardis/io/parsers/arepo.py 66.35% <0.00%> (-2.70%) ⬇️
.../montecarlo/montecarlo_numba/single_packet_loop.py 25.75% <0.00%> (-1.11%) ⬇️
tardis/transport/r_packet_transport.py 17.97% <3.44%> (-2.03%) ⬇️
...dis/montecarlo/montecarlo_numba/numba_interface.py 36.36% <57.14%> (+1.71%) ⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@wkerzendorf
Copy link
Member

wait with merge until @code-explorer is done.

@andrewfullard andrewfullard marked this pull request as draft March 15, 2024 14:19
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.

4 participants