tardis-sn / tardis

TARDIS - Temperature And Radiative Diffusion In Supernovae
https://tardis-sn.github.io/tardis
202 stars 405 forks source link

Add molecular data exposure to tardis atom data #2806

Closed jvshields closed 2 months ago

jvshields commented 2 months ago

Needed for STARDIS to access the molecular information ingested by carsus added in https://github.com/tardis-sn/carsus/pull/396

Currently I don't think it's worth adding a molecular_data class, but if we ever want to do more complex handling of molecules, calculating them in a similar way to atomic number densities instead of just using equilibrium constants, then it might be worth reconsidering.

tardis-bot commented 2 months ago

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

```diff 5 I001 [*] Import block is un-sorted or un-formatted 4 G004 [ ] Logging statement uses f-string 3 F405 [ ] `deepcopy` may be undefined, or defined from star imports 2 RET505 [ ] Unnecessary `else` after `return` statement 2 D202 [*] No blank lines allowed after function docstring (found 1) 1 G001 [ ] Logging statement uses `str.format` 1 PT020 [ ] `@pytest.yield_fixture` is deprecated, use `@pytest.fixture` 1 RET506 [ ] Unnecessary `elif` after `raise` statement 1 E902 [ ] No such file or directory (os error 2) 1 F401 [*] `astropy.units` imported but unused 1 F403 [ ] `from tardis.tests.fixtures.atom_data import *` used; unable to detect undefined names 1 F821 [ ] Undefined name `pytest_report_header` 1 PGH004 [ ] Use specific rule codes when using `noqa` 1 UP030 [*] Use implicit references for positional format fields ```

Complete output(might be large):

```diff tardis/conftest.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/conftest.py:11:1: F403 `from tardis.tests.fixtures.atom_data import *` used; unable to detect undefined names tardis/conftest.py:30:49: PGH004 Use specific rule codes when using `noqa` tardis/conftest.py:32:9: F821 Undefined name `pytest_report_header` tardis/conftest.py:145:5: RET505 Unnecessary `else` after `return` statement tardis/conftest.py:169:1: PT020 `@pytest.yield_fixture` is deprecated, use `@pytest.fixture` tardis/conftest.py:240:19: F405 `deepcopy` may be undefined, or defined from star imports tardis/conftest.py:249:19: F405 `deepcopy` may be undefined, or defined from star imports tardis/conftest.py:278:19: F405 `deepcopy` may be undefined, or defined from star imports tardis/gui/tests/test_gui.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/gui/tests/test_gui.py:5:25: F401 [*] `astropy.units` imported but unused tardis/gui/tests/test_gui.py:8:1: I001 [*] Import block is un-sorted or un-formatted tardis/io/atom_data/base.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/io/atom_data/base.py:198:34: G004 Logging statement uses f-string tardis/io/atom_data/base.py:263:17: G004 Logging statement uses f-string tardis/io/atom_data/base.py:267:21: UP030 Use implicit references for positional format fields tardis/io/atom_data/base.py:267:21: G001 Logging statement uses `str.format` tardis/io/atom_data/base.py:704:17: G004 Logging statement uses f-string tardis/tests/test_tardis_full_formal_integral.py:61:9: RET505 Unnecessary `else` after `return` statement tardis/visualization/tools/liv_plot.py:1:1: I001 [*] Import block is un-sorted or un-formatted tardis/visualization/tools/liv_plot.py:25:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/visualization/tools/liv_plot.py:48:9: D202 [*] No blank lines allowed after function docstring (found 1) tardis/visualization/tools/liv_plot.py:247:17: G004 Logging statement uses f-string tardis/visualization/tools/liv_plot.py:357:13: RET506 Unnecessary `elif` after `raise` statement tardis/visualization/tools/tests/test_liv_plot.py:1:1: E902 No such file or directory (os error 2) Found 25 errors. [*] 8 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option). ```
andrewfullard commented 2 months ago

Seems reasonable. Should definitely become a class eventually.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.79%. Comparing base (5fe6806) to head (ec4d62a). Report is 3 commits behind head on master.

Files Patch % Lines
tardis/io/atom_data/base.py 70.00% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2806 +/- ## ========================================== + Coverage 69.88% 70.79% +0.91% ========================================== Files 208 209 +1 Lines 15453 15638 +185 ========================================== + Hits 10799 11071 +272 + Misses 4654 4567 -87 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tardis-bot commented 2 months ago

*beep* *bop* Hi human, I ran benchmarks as you asked comparing master (5fe6806d2dc18afa155ffeb3a3cb31518637a214) and the latest commit (ec4d62ad7b17332ad72001f315e642ade5bfd9ec). Here are the logs produced by ASV. Results can also be downloaded as artifacts here.

Significantly changed benchmarks:

```diff ```

All benchmarks:

```diff Benchmarks that have stayed the same: | Change | Before [b6f11851] | After [ec4d62ad] | Ratio | Benchmark (Parameter) | |----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------| | | 491±100ns | 1.09±0.3μs | ~2.22 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation | | | 2.87±0.5μs | 3.24±0.3μs | ~1.13 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket | | | 601±200ns | 521±100ns | ~0.87 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation | | | 28.8±8μs | 24.8±7μs | ~0.86 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list | | | 2.33±1μs | 1.98±2μs | ~0.85 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators | | | 3.92±0.6μs | 3.20±0.3μs | ~0.82 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell | | | 63.6±0.06ms | 69.1±2ms | 1.09 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe | | | 6.05±0.9μs | 6.37±0.8μs | 1.05 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket | | | 39.3±0.02s | 39.2±0.07s | 1.00 | run_tardis.BenchmarkRunTardis.time_run_tardis | | | 2.07±0m | 2.07±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions | | | 1.44±0.3μs | 1.44±0.4μs | 1.00 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line | | | 1.05±0m | 1.04±0m | 0.99 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking | | | 211±0.1ns | 208±0.3ns | 0.99 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body | | | 735±0.2ns | 728±5ns | 0.99 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter | | | 30.7±0.03μs | 30.4±0.05μs | 0.99 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list | | | 4.16±0.04ms | 4.09±0.03ms | 0.98 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') | | | 1.73±0.02ms | 1.70±0.02ms | 0.98 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop | | | 7.40±2μs | 7.28±2μs | 0.98 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley | | | 581±200ns | 561±200ns | 0.97 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation | | | 2.69±0.4ms | 2.60±0.5ms | 0.97 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop | | | 1.24±0μs | 1.19±0.1μs | 0.96 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary | | | 44.0±20μs | 42.3±20μs | 0.96 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission | | | 46.8±20μs | 44.1±30μs | 0.94 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter | | | 2.95±0.02ms | 2.71±0.02ms | 0.92 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') | ```

If you want to see the graph of the results, you can check it here

wkerzendorf commented 2 months ago

Seems reasonable. Should definitely become a class eventually.

I think make it a dataclass now to put all of these together in one.