Skip to content

Conversation

@wkerzendorf
Copy link
Member

This pull request refactors the Monte Carlo transport benchmarking and core transport modules to reorganize packet-related code into a new packets submodule and moves and modernizes the Numba opacity state implementation. It also updates imports and references throughout the codebase to reflect these structural changes, improving code clarity and maintainability.

Codebase Structure Refactoring

  • All packet-related classes and functions such as RPacket, PacketCollection, packet_trackers, and VPacketCollection have been moved from tardis.transport.montecarlo and related files into the new tardis.transport.montecarlo.packets submodule. All relevant imports and references have been updated in benchmarks and core modules. [1] [2] [3] [4] [5] [6]
  • The virtual packet code previously in tardis.transport.montecarlo.vpacket is now in tardis.transport.montecarlo.packets.virtual_packet, and all usages have been updated accordingly. [1] [2] [3] [4] [5] [6] [7]

Numba Opacity State Refactor

  • The Numba-compatible opacity state implementation has been moved from tardis.transport.montecarlo.numba_interface.py to tardis/opacities/opacity_state_numba.py, renamed as OpacityStateNumba, and rewritten to use modern Python type annotations and improved docstrings. The initialization function is now called opacity_state_numba_initialize. [1] [2] [3] [4] [5] [6] [7]
  • All code referencing the old opacity_state_initialize has been updated to use opacity_state_numba_initialize.

Macro Atom and Interaction Types

  • The LineInteractionType enum has been moved out of the Numba interface and into tardis.transport.montecarlo.interaction.py for better separation of concerns. All references have been updated. [1] [2] [3] [4]

These changes collectively improve the modularity and clarity of the Monte Carlo transport code, making it easier to maintain and extend.### 📝 Description

Type: 🪲 bugfix | 🚀 feature | ☣️ breaking change | 🚦 testing | 📝 documentation | 🎢 infrastructure

Write a complete description of your changes, including the necessary context or any piece of information required to understand your work.

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

📌 Resources

Examples, notebooks, and links to useful references.

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

…ment RPacket and VPacket with associated methods
- Moved packet-related classes and functions into a new 'packets' submodule for better organization.
- Updated import statements throughout the codebase to reflect the new structure.
- Enhanced type annotations and docstrings for clarity and improved code readability.
- Refactored the PacketCollection, LastInteractionTracker, RPacket, VPacket, and associated tracker classes to utilize Numba's JIT compilation more effectively.
- Improved the handling of packet properties and interactions, ensuring consistency across the Monte Carlo transport processes.
- Updated tests to accommodate the new structure and ensure all functionalities remain intact.
Copilot AI review requested due to automatic review settings August 26, 2025 22:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the Monte Carlo transport code structure by moving packet-related classes and functions into a dedicated packets submodule and modernizes the Numba opacity state implementation. The changes improve code organization and maintainability without altering core functionality.

  • Reorganizes packet-related code (RPacket, VPacket, PacketCollection, packet_trackers) from tardis.transport.montecarlo into the new tardis.transport.montecarlo.packets submodule
  • Moves and modernizes the Numba opacity state from numba_interface.py to tardis/opacities/opacity_state_numba.py with updated type annotations and improved documentation
  • Relocates LineInteractionType enum from the Numba interface to the interaction module for better separation of concerns

Reviewed Changes

Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tardis/transport/montecarlo/packets/radiative_packet.py New location for RPacket class with modern type annotations
tardis/transport/montecarlo/packets/virtual_packet.py New location for VPacket class with improved documentation
tardis/transport/montecarlo/packets/packet_trackers.py New location for packet tracking classes with type annotations
tardis/transport/montecarlo/packets/packet_collections.py New location for packet collections with enhanced docstrings
tardis/opacities/opacity_state_numba.py Modernized opacity state implementation with comprehensive documentation
tardis/transport/montecarlo/interaction.py Added LineInteractionType enum definition
Multiple test and benchmark files Updated imports to reflect new module structure

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@tardis-bot
Copy link
Contributor

tardis-bot commented Aug 26, 2025

*beep* *bop*
Hi human,
I ran ruff on the latest commit (ede4621).
Here are the outputs produced.
Results can also be downloaded as artifacts here.
Summarised output:

Details
14	F821  	[ ] undefined-name
7	E402  	[ ] module-import-not-at-top-of-file
7	I001  	[*] unsorted-imports
6	F401  	[ ] unused-import
2	B019  	[ ] cached-instance-method
2	B027  	[ ] empty-method-without-abstract-decorator
1	E902  	[ ] io-error
1	W293  	[ ] blank-line-with-whitespace
1	F811  	[ ] redefined-while-unused
1	PT021 	[ ] pytest-fixture-finalizer-callback
1	RET505	[*] superfluous-else-return
1	RET506	[*] superfluous-else-raise
1	RET508	[*] superfluous-else-break
Found 45 errors.
[*] 14 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

Complete output(might be large):

Details
benchmarks/benchmark_base.py:1:1: I001 [*] Import block is un-sorted or un-formatted
benchmarks/benchmark_base.py:75:9: RET506 [*] Unnecessary `else` after `raise` statement
benchmarks/transport_montecarlo_packet_trackers.py:6:40: F401 [*] `asv_runner.benchmarks.mark.parameterize` imported but unused
benchmarks/transport_montecarlo_packet_trackers.py:19:5: B019 Use of `functools.lru_cache` or `functools.cache` on methods can lead to memory leaks
benchmarks/transport_montecarlo_vpacket.py:54:5: B019 Use of `functools.lru_cache` or `functools.cache` on methods can lead to memory leaks
tardis/transport/montecarlo/__init__.py:14:1: E402 Module level import not at top of file
tardis/transport/montecarlo/__init__.py:24:1: E402 Module level import not at top of file
tardis/transport/montecarlo/__init__.py:25:5: F401 `tardis.transport.montecarlo.packets.packet_collections.PacketCollection` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
tardis/transport/montecarlo/__init__.py:27:1: E402 Module level import not at top of file
tardis/transport/montecarlo/__init__.py:27:66: F401 `tardis.transport.montecarlo.packets.radiative_packet.RPacket` imported but unused; consider removing, adding to `__all__`, or using a redundant alias
tardis/transport/montecarlo/base.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/base.py:10:56: F401 [*] `tardis.opacities.continuum.continuum_state.ContinuumState` imported but unused
tardis/transport/montecarlo/base.py:12:5: F401 [*] `tardis.opacities.opacity_state.OpacityState` imported but unused
tardis/transport/montecarlo/packet_source.py:45:5: B027 `BasePacketSource.create_packet_radii` is an empty method in an abstract base class, but has no abstract decorator
tardis/transport/montecarlo/packet_source.py:48:5: B027 `BasePacketSource.create_packet_velocities` is an empty method in an abstract base class, but has no abstract decorator
tardis/transport/montecarlo/packet_source.py:249:9: RET505 [*] Unnecessary `else` after `return` statement
tardis/transport/montecarlo/packets/packet_collections.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/packets/packet_trackers.py:250:1: W293 Blank line contains whitespace
tardis/transport/montecarlo/packets/radiative_packet.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/packets/tests/test_packet.py:21:1: E402 Module level import not at top of file
tardis/transport/montecarlo/packets/tests/test_vpacket.py:12:1: E402 Module level import not at top of file
tardis/transport/montecarlo/packets/virtual_packet.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/r_packet.py:1:1: E902 No such file or directory (os error 2)
tardis/transport/montecarlo/r_packet_transport.py:115:13: RET508 [*] Unnecessary `elif` after `break` statement
tardis/transport/montecarlo/single_packet_loop.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/tests/conftest.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/transport/montecarlo/tests/conftest.py:7:44: F401 [*] `tardis.opacities.opacity_state.opacity_state_initialize` imported but unused
tardis/transport/montecarlo/tests/test_montecarlo.py:26:1: E402 Module level import not at top of file
tardis/transport/montecarlo/tests/test_montecarlo.py:31:1: E402 Module level import not at top of file
tardis/transport/montecarlo/tests/test_montecarlo.py:47:5: PT021 Use `yield` instead of `request.addfinalizer`
tardis/transport/montecarlo/tests/test_montecarlo.py:104:5: F811 Redefinition of unused `test_get_random_mu_different_output` from line 95
tardis/transport/montecarlo/tests/test_montecarlo.py:384:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:470:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:471:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:479:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:496:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:501:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:524:9: F821 Undefined name `transport`
tardis/transport/montecarlo/tests/test_montecarlo.py:525:9: F821 Undefined name `transport`
tardis/transport/montecarlo/tests/test_montecarlo.py:526:9: F821 Undefined name `transport`
tardis/transport/montecarlo/tests/test_montecarlo.py:527:9: F821 Undefined name `transport`
tardis/transport/montecarlo/tests/test_montecarlo.py:529:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:542:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:566:5: F821 Undefined name `mc`
tardis/transport/montecarlo/tests/test_montecarlo.py:579:5: F821 Undefined name `mc`
Found 45 errors.
[*] 14 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

andrewfullard
andrewfullard previously approved these changes Aug 27, 2025
Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

Now might be a good time to add docstrings to the files that have been moved.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

@andrewfullard andrewfullard self-requested a review August 27, 2025 18:18
@wkerzendorf wkerzendorf added the git-lfs-pull Allow git lfs pull in PRs label Aug 27, 2025
@github-actions github-actions bot removed the git-lfs-pull Allow git lfs pull in PRs label Aug 27, 2025
@codecov
Copy link

codecov bot commented Aug 27, 2025

Codecov Report

❌ Patch coverage is 67.94258% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.98%. Comparing base (995879b) to head (ede4621).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...s/transport/montecarlo/packets/radiative_packet.py 48.71% 40 Missing ⚠️
...rdis/transport/montecarlo/tests/test_montecarlo.py 7.14% 26 Missing ⚠️
...transport/montecarlo/packets/tests/test_vpacket.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3266      +/-   ##
==========================================
- Coverage   74.14%   73.98%   -0.16%     
==========================================
  Files         259      259              
  Lines       17886    17973      +87     
==========================================
+ Hits        13261    13297      +36     
- Misses       4625     4676      +51     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@andrewfullard andrewfullard merged commit 233501c into tardis-sn:master Aug 27, 2025
20 of 24 checks passed
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.

3 participants