V inner formal integral #2800
Open
V inner formal integral #2800
+638
−40
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
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)
…wfullard/tardis into v_inner_solver_restructure
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
*beep* *bop* 8 G004 [ ] Logging statement uses f-string
4 RET505 [ ] Unnecessary `else` after `return` statement
2 PTH117 [ ] `os.path.isabs()` should be replaced by `Path.is_absolute()`
2 I001 [*] Import block is un-sorted or un-formatted
1 RET506 [ ] Unnecessary `else` after `raise` statement
1 TD005 [ ] Missing issue description after `TODO`
1 E999 [ ] SyntaxError: Expected an expression
1 D202 [*] No blank lines allowed after function docstring (found 1)
1 F541 [*] f-string without any placeholders
1 F811 [ ] Redefinition of unused `opacity_state_initialize` from line 13
Complete output(might be large): docs/physics/setup/model.ipynb:cell 2:2:1: I001 [*] Import block is un-sorted or un-formatted
docs/physics/setup/model.ipynb:cell 25:19:12: F541 [*] f-string without any placeholders
docs/workflows/v_inner_solver_workflow.ipynb:cell 1:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/io/configuration/config_reader.py:53:29: G004 Logging statement uses f-string
tardis/io/configuration/config_reader.py:117:9: RET505 Unnecessary `else` after `return` statement
tardis/io/configuration/config_reader.py:141:13: RET505 Unnecessary `else` after `return` statement
tardis/io/configuration/config_reader.py:218:29: G004 Logging statement uses f-string
tardis/io/configuration/schemas/montecarlo_definitions.yml:1:13: E999 SyntaxError: Expected an expression
tardis/io/model/parse_geometry_configuration.py:50:12: PTH117 `os.path.isabs()` should be replaced by `Path.is_absolute()`
tardis/model/base.py:340:12: PTH117 `os.path.isabs()` should be replaced by `Path.is_absolute()`
tardis/model/base.py:375:21: G004 Logging statement uses f-string
tardis/spectrum/formal_integral.py:23:5: F811 Redefinition of unused `opacity_state_initialize` from line 13
tardis/spectrum/formal_integral.py:340:13: RET506 Unnecessary `else` after `raise` statement
tardis/spectrum/formal_integral.py:405:9: D202 [*] No blank lines allowed after function docstring (found 1)
tardis/spectrum/formal_integral.py:700:5: RET505 Unnecessary `else` after `return` statement
tardis/spectrum/formal_integral.py:736:5: RET505 Unnecessary `else` after `return` statement
tardis/workflows/simple_tardis_workflow.py:218:17: G004 Logging statement uses f-string
tardis/workflows/simple_tardis_workflow.py:432:17: G004 Logging statement uses f-string
tardis/workflows/v_inner_solver.py:16:3: TD005 Missing issue description after `TODO`
tardis/workflows/v_inner_solver.py:175:17: G004 Logging statement uses f-string
tardis/workflows/v_inner_solver.py:202:17: G004 Logging statement uses f-string
tardis/workflows/v_inner_solver.py:310:17: G004 Logging statement uses f-string
Found 22 errors.
[*] 4 fixable with the `--fix` option.
|
|
*beep* *bop* Hi, human. The Click here to see your results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2800 +/- ##
==========================================
- Coverage 70.22% 69.23% -0.99%
==========================================
Files 214 216 +2
Lines 15981 16140 +159
==========================================
- Hits 11223 11175 -48
- Misses 4758 4965 +207 ☔ View full report in Codecov by Sentry. |
…-/tardis into v_inner_formal_integral
|
*beep* *bop* Significantly changed benchmarks: All benchmarks: Benchmarks that have stayed the same:
| Change | Before [83282e3c] <master> | After [e6b2f232] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
| | 2.59±1μs | 2.20±1μs | ~0.85 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators |
| | 3.18±0.3μs | 2.71±0.5μs | ~0.85 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket |
| | 36.7±3μs | 30.9±0μs | ~0.84 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list |
| | 31.0±10μs | 22.9±6μs | ~0.74 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
| | 2.39±0.4ms | 2.62±0.4ms | 1.10 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop |
| | 40.7±20μs | 42.6±20μs | 1.05 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission |
| | 3.68±0.03ms | 3.78±0.05ms | 1.03 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') |
| | 202±0.1ns | 207±1ns | 1.03 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body |
| | 62.0±0.1ms | 63.7±0ms | 1.03 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe |
| | 1.05±0m | 1.07±0m | 1.02 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking |
| | 39.9±0.04s | 40.3±0.04s | 1.01 | run_tardis.BenchmarkRunTardis.time_run_tardis |
| | 1.18±0.01μs | 1.19±0μs | 1.01 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary |
| | 2.07±0m | 2.07±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions |
| | 49.3±30μs | 49.2±20μs | 1.00 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter |
| | 1.59±0.3μs | 1.58±0.4μs | 0.99 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line |
| | 561±200ns | 551±100ns | 0.98 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation |
| | 581±200ns | 561±100ns | 0.97 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation |
| | 2.98±0ms | 2.88±0.02ms | 0.97 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') |
| | 746±0.07ns | 727±0.5ns | 0.97 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter |
| | 7.08±1μs | 6.89±2μs | 0.97 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley |
| | 6.30±0.8μs | 5.90±0.8μs | 0.94 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket |
| | 3.41±0.6μs | 3.21±0.5μs | 0.94 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell |
| | 571±100ns | 531±100ns | 0.93 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation |
| | 1.82±0.02ms | 1.67±0.01ms | 0.92 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
If you want to see the graph of the results, you can check it here |
|
@Rodot- this needs to be rebased now |
andrewfullard
previously approved these changes
Nov 13, 2024
6 tasks
andrewfullard
approved these changes
Nov 13, 2024
jvshields
reviewed
Dec 2, 2024
| c = const.c.cgs.value | ||
| kb = const.k_B.cgs.value | ||
|
|
||
| def B(nu, T): |
There was a problem hiding this comment.
Probably we can import the blackbody function from somewhere else in tardis.
jvshields
reviewed
Dec 2, 2024
| ct = simulation_state.time_explosion.cgs.value * const.c.cgs.value | ||
| t_rad = simulation_state.radiation_field_state.temperature.cgs.value | ||
|
|
||
| h = const.h.cgs.value |
There was a problem hiding this comment.
Not a fan of these three values being saved like this in this function.
jvshields
reviewed
Dec 2, 2024
|
|
||
| return estimates | ||
|
|
||
| def reproject(self, a1, m1, a2, m2): |
There was a problem hiding this comment.
Not sure if this is would be used elsewhere, but it seems like quite a generic function to live in this class. Maybe this should live in util.py?
Also, a1, m1, a2, and m2 could be a little more explicit. Just named "array1" and "mask1".
jvshields
reviewed
Dec 2, 2024
There was a problem hiding this comment.
This notebook is okay, but if it serves as the documentation for this workflow, it'd be nice to have a bit more explanation of what the workflow is, why you'd want to use it, and how you'd want to use it.
jvshields
reviewed
Dec 2, 2024
| freqs = freqs[order] | ||
| taus = plasma.tau_sobolevs.values[order] | ||
|
|
||
| extra = bin_size - len(freqs) % bin_size |
There was a problem hiding this comment.
I can't tell what the point of this bit of code is. Could there be more informative names, or at least a comment on why you need to handle the extra frequency bins like this?
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.
📝
Fixes the formal integral to handle changes in geometry
Type: 🪲
bugfixWhen the formal integral is run with the v_inner solver, if the final active geometry state is different than that of the initial geometry mismatches in plasma and opacity quantities occur. This PR rectifies this by slicing these arrays to the correct size
Depends on PR #2797
🚦 Testing
How did you test these changes?
CUDA version of the formal integral was tested locally
☑️ Checklist
build_docslabel