Simulation solver workflow #2730
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2730 +/- ##
==========================================
- Coverage 69.91% 68.31% -1.60%
==========================================
Files 206 209 +3
Lines 15523 15763 +240
==========================================
- Hits 10853 10769 -84
- Misses 4670 4994 +324 ☔ View full report in Codecov by Sentry. |
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
*beep* *bop* Hi, human. The Click here to see the build log. |
andrewfullard
commented
Jul 24, 2024
|
|
||
|
|
||
| class StandardSimulationSolver: | ||
| def __init__(self, configuration): |
There was a problem hiding this comment.
If we want this to replace the full run_tardis etc it should probably be able to take in a configuration path and convert it to a configuration object. Probably as the only option.
andrewfullard
commented
Jul 24, 2024
andrewfullard
commented
Jul 24, 2024
| def solve_simulation_state( | ||
| self, | ||
| estimated_values, | ||
| ): |
There was a problem hiding this comment.
Given that we are in the simulation solver class, perhaps this should just be in solve()
andrewfullard
commented
Jul 24, 2024
| if u.isclose( | ||
| configuration.supernova.luminosity_wavelength_start, 0 * u.angstrom | ||
| ): | ||
| self.luminosity_nu_end = np.inf * u.Hz |
There was a problem hiding this comment.
This is a mess that should be handled by the configuration
andrewfullard
commented
Jul 24, 2024
| ): | ||
| final_iteration_packet_count = self.real_packet_count | ||
|
|
||
| self.final_iteration_packet_count = int(final_iteration_packet_count) |
There was a problem hiding this comment.
This is a mess that should be handled by the configuration
andrewfullard
commented
Jul 24, 2024
| ).to(u.Hz) | ||
|
|
||
| # montecarlo settings | ||
| self.total_iterations = int(configuration.montecarlo.iterations) |
There was a problem hiding this comment.
Casting to int should be handled by the configuration parser
andrewfullard
commented
Jul 24, 2024
| ) | ||
|
|
||
| # spectrum settings | ||
| self.integrated_spectrum_settings = configuration.spectrum.integrated |
There was a problem hiding this comment.
This is weird.
andrewfullard
commented
Jul 24, 2024
| current_value = getattr(self.simulation_state, key) | ||
| estimated_value = estimated_values[key] | ||
| no_of_shells = ( | ||
| self.simulation_state.no_of_shells if key != "t_inner" else 1 |
There was a problem hiding this comment.
This gets a bit messy due to the "special" status of t_inner in the convergence and configuration. Probably unavoidable without extremely rigid requirements on the config.
andrewfullard
commented
Jul 24, 2024
| "t_inner" | ||
| ] | ||
|
|
||
| def solve_plasma( |
There was a problem hiding this comment.
This mess should be cleaned up with plasma restructures
andrewfullard
commented
Jul 24, 2024
| iteration=self.completed_iterations, | ||
| ) | ||
|
|
||
| virtual_packet_energies = self.transport_solver.run( |
There was a problem hiding this comment.
Should be .solve
andrewfullard
commented
Jul 24, 2024
|
|
||
| return transport_state, virtual_packet_energies | ||
|
|
||
| def initialize_spectrum_solver( |
There was a problem hiding this comment.
This is a mess and should probably be handled by the spectrum solver itself.
andrewfullard
commented
Jul 24, 2024
| transport_state, virtual_packet_energies = self.solve_montecarlo( | ||
| self.final_iteration_packet_count, self.virtual_packet_count | ||
| ) | ||
| self.initialize_spectrum_solver( |
There was a problem hiding this comment.
Weird to be init at the end.
|
*beep* *bop* 9 W291 [*] Trailing whitespace
7 G004 [ ] Logging statement uses f-string
4 W605 [*] Invalid escape sequence: `\A`
3 F631 [ ] Assert test is a non-empty tuple, which is always `True`
2 I001 [*] Import block is un-sorted or un-formatted
1 ANN205 [ ] Missing return type annotation for staticmethod `get_relative_path`
1 B018 [ ] Found useless expression. Either assign it to a variable or remove it.
1 COM818 [ ] Trailing comma on bare tuple prohibited
1 INP001 [ ] File `tardis/spectrum/tests/test_spectrum_solver.py` is part of an implicit namespace package. Add an `__init__.py`.
1 RET506 [ ] Unnecessary `else` after `raise` statement
1 E999 [ ] SyntaxError: Expected a statement
1 PLW0127 [ ] Self-assignment of variable `observed_spectrum`
Complete output(might be large): asv.conf.json:1:1: B018 Found useless expression. Either assign it to a variable or remove it.
benchmarks/benchmark_base.py:29:9: ANN205 Missing return type annotation for staticmethod `get_relative_path`
benchmarks/benchmark_base.py:75:9: RET506 Unnecessary `else` after `raise` statement
docs/contributing/development/running_tests.rst:1:1: E999 SyntaxError: Expected a statement
docs/contributing/development/running_tests.rst:8:90: COM818 Trailing comma on bare tuple prohibited
docs/contributing/development/running_tests.rst:41:42: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:64:112: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:65:124: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:69:92: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:70:107: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:76:102: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:78:116: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:80:124: W291 [*] Trailing whitespace
docs/contributing/development/running_tests.rst:92:62: W291 [*] Trailing whitespace
docs/workflows/simple_workflow.ipynb:cell 1:1:1: I001 [*] Import block is un-sorted or un-formatted
docs/workflows/simple_workflow.ipynb:cell 7:10:26: W605 [*] Invalid escape sequence: `\A`
docs/workflows/simple_workflow.ipynb:cell 7:11:40: W605 [*] Invalid escape sequence: `\A`
docs/workflows/standard_workflow.ipynb:cell 1:1:1: I001 [*] Import block is un-sorted or un-formatted
docs/workflows/standard_workflow.ipynb:cell 7:10:26: W605 [*] Invalid escape sequence: `\A`
docs/workflows/standard_workflow.ipynb:cell 7:11:40: W605 [*] Invalid escape sequence: `\A`
tardis/spectrum/tests/test_spectrum_solver.py:1:1: INP001 File `tardis/spectrum/tests/test_spectrum_solver.py` is part of an implicit namespace package. Add an `__init__.py`.
tardis/visualization/tools/tests/test_sdec_plot.py:343:17: F631 Assert test is a non-empty tuple, which is always `True`
tardis/visualization/tools/tests/test_sdec_plot.py:475:13: F631 Assert test is a non-empty tuple, which is always `True`
tardis/visualization/tools/tests/test_sdec_plot.py:557:13: PLW0127 Self-assignment of variable `observed_spectrum`
tardis/visualization/tools/tests/test_sdec_plot.py:618:13: F631 Assert test is a non-empty tuple, which is always `True`
tardis/workflows/simple_simulation.py:149:21: G004 Logging statement uses f-string
tardis/workflows/simple_simulation.py:252:17: G004 Logging statement uses f-string
tardis/workflows/simple_simulation.py:427:17: G004 Logging statement uses f-string
tardis/workflows/standard_simulation.py:176:13: G004 Logging statement uses f-string
tardis/workflows/standard_simulation.py:216:17: G004 Logging statement uses f-string
tardis/workflows/workflow_logging.py:69:13: G004 Logging statement uses f-string
tardis/workflows/workflow_logging.py:107:21: G004 Logging statement uses f-string
Found 32 errors.
[*] 15 fixable with the `--fix` option.
|
|
*beep* *bop* Significantly changed benchmarks: | Change | Before [1f5ca78f] <master> | After [79df0e31] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|---------------------------------------------------------------------------------------------------------|
| - | 1.83±0ms | 1.66±0ms | 0.91 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
All benchmarks: Benchmarks that have improved:
| Change | Before [1f5ca78f] <master> | After [79df0e31] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|---------------------------------------------------------------------------------------------------------|
| - | 1.83±0ms | 1.66±0ms | 0.91 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
Benchmarks that have stayed the same:
| Change | Before [1f5ca78f] <master> | After [79df0e31] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
| | 1.95±1μs | 2.43±2μs | ~1.25 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators |
| | 3.56±0ms | 4.05±0.01ms | ~1.14 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') |
| | 6.83±2μs | 7.74±2μs | ~1.13 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley |
| | 1.56±0.5μs | 1.74±0.3μs | ~1.12 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line |
| | 631±100ns | 561±100ns | ~0.89 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation |
| | 631±100ns | 561±100ns | ~0.89 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation |
| | 581±90ns | 491±300ns | ~0.85 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation |
| | 46.3±20μs | 50.9±30μs | 1.10 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter |
| | 2.65±0.4ms | 2.72±0.4ms | 1.03 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop |
| | 46.0±20μs | 46.8±20μs | 1.02 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission |
| | 727±0.1ns | 731±0.5ns | 1.01 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter |
| | 3.01±0.01ms | 3.01±0.01ms | 1.00 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') |
| | 39.5±0.1s | 39.6±0s | 1.00 | run_tardis.BenchmarkRunTardis.time_run_tardis |
| | 1.06±0m | 1.06±0m | 1.00 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking |
| | 2.07±0m | 2.08±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions |
| | 203±0.2ns | 203±0.04ns | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body |
| | 1.20±0μs | 1.19±0μs | 0.99 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary |
| | 3.60±0.4μs | 3.49±0.2μs | 0.97 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket |
| | 6.31±0.9μs | 6.11±0.8μs | 0.97 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket |
| | 66.6±0.2ms | 64.1±0.1ms | 0.96 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe |
| | 3.60±0.3μs | 3.37±0.3μs | 0.94 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell |
| | 24.2±7μs | 22.6±6μs | 0.93 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
| | 32.8±1μs | 30.5±0.01μs | 0.93 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list |
If you want to see the graph of the results, you can check it here |
|
Have the tests been run recently? I don't think the notebook should be able to run as there is no opacity state passed to the transport solver |
|
Unless I'm missing something, I believe this needs to be rebased/merge in master |
Solvers pass info to each other Simplified some parts
Also adds docstrings to methods. Updates some methods to use new functionality of the plasma. Adds requirements for the convergence plots (still broken)
andrewfullard
force-pushed
the
simulation-solver-workflow
branch
from
3a24f98
to
a68ebaa
Compare
Rodot-
reviewed
Aug 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Description
Type: 🚀
featureDuplicates the existing simulation solver as a hopefully clean workflow setup.
🚦 Testing
How did you test these changes?
☑️ Checklist
build_docslabel