-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
Simple Atom data refactor #2783
Simple Atom data refactor #2783
Conversation
*beep* *bop* 3 G004 [ ] Logging statement uses f-string
1 G001 [ ] Logging statement uses `str.format`
1 E902 [ ] No such file or directory (os error 2)
1 UP030 [*] Use implicit references for positional format fields
Complete output(might be large): tardis/io/atom_data/base.py:182:34: G004 Logging statement uses f-string
tardis/io/atom_data/base.py:236:17: G004 Logging statement uses f-string
tardis/io/atom_data/base.py:240:21: UP030 Use implicit references for positional format fields
tardis/io/atom_data/base.py:240:21: G001 Logging statement uses `str.format`
tardis/io/atom_data/base.py:670:17: G004 Logging statement uses f-string
tardis/plasma/nlte/__init__.py:1:1: E902 No such file or directory (os error 2)
Found 6 errors.
No fixes available (1 hidden fix can be enabled with the `--unsafe-fixes` option).
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2783 +/- ##
==========================================
- Coverage 69.91% 69.68% -0.24%
==========================================
Files 206 208 +2
Lines 15523 15490 -33
==========================================
- Hits 10853 10794 -59
- Misses 4670 4696 +26 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you expand a bit on the broader design goals of this PR?
*beep* *bop* Significantly changed benchmarks: All benchmarks: Benchmarks that have stayed the same:
| Change | Before [8a317dd9] <master> | After [a0a15666] | Ratio | Benchmark (Parameter) |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
| | 6.75±0.6μs | 6.05±1μs | ~0.90 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket |
| | 551±100ns | 491±100ns | ~0.89 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation |
| | 52.0±20μs | 45.9±20μs | ~0.88 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter |
| | 48.8±20μs | 42.4±20μs | ~0.87 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission |
| | 671±200ns | 551±100ns | ~0.82 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation |
| | 3.07±0.5μs | 3.36±0.5μs | 1.09 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell |
| | 7.13±2μs | 7.60±1μs | 1.07 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley |
| | 1.64±0.2μs | 1.72±0.3μs | 1.05 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line |
| | 769±0.7ns | 801±2ns | 1.04 | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter |
| | 2.12±2μs | 2.15±1μs | 1.01 | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators |
| | 1.05±0m | 1.04±0m | 1.00 | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking |
| | 2.07±0m | 2.07±0m | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions |
| | 206±0.5ns | 207±0.1ns | 1.00 | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body |
| | 30.4±0.06μs | 30.4±0.07μs | 1.00 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list |
| | 39.3±0.07s | 39.0±0.02s | 0.99 | run_tardis.BenchmarkRunTardis.time_run_tardis |
| | 2.58±0.5ms | 2.56±0.5ms | 0.99 | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop |
| | 1.69±0.01ms | 1.66±0ms | 0.98 | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop |
| | 64.1±0ms | 63.0±0.3ms | 0.98 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe |
| | 3.72±0.01ms | 3.61±0.01ms | 0.97 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom') |
| | 1.21±0.04μs | 1.16±0μs | 0.96 | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary |
| | 571±200ns | 541±100ns | 0.95 | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation |
| | 2.79±0.02ms | 2.64±0.01ms | 0.95 | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter') |
| | 23.5±6μs | 22.2±5μs | 0.94 | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
| | 2.99±0.4μs | 2.79±0.7μs | 0.93 | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket |
If you want to see the graph of the results, you can check it here |
) | ||
|
||
cmfgen_collision_data = CMFGENCollisionData( | ||
yg_data, collision_data_temperatures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is quite right. I'm pretty sure the yg_data stores it as part of the columns. Or ChiantiCollisionData. have a quick look in the plasma
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the plasma YgData
class, it uses both yg_data
and collision_data_temperatures
. See https://github.com/andrewfullard/tardis/blob/atom-data-refactor-v2/tardis/plasma/properties/atomic.py#L676 and https://github.com/andrewfullard/tardis/blob/atom-data-refactor-v2/tardis/plasma/properties/atomic.py#L687. For the Chianti-source collision_data, it is used in NLTEData, see https://github.com/andrewfullard/tardis/blob/atom-data-refactor-v2/tardis/io/atom_data/nlte_data.py#L18-L19 and https://github.com/andrewfullard/tardis/blob/atom-data-refactor-v2/tardis/io/atom_data/nlte_data.py#L86.
Oddly in the from_hdf
method in this class, collision_data
is set to "dummy value" if the data is sourced from CMFGen, see https://github.com/andrewfullard/tardis/blob/atom-data-refactor-v2/tardis/io/atom_data/base.py#L199. That is why I did not include it in the CMFGen dataclass.
24d320e
to
86a4667
Compare
📝 Description
Type: 🎢
infrastructure
This uses dataclasses to begin simplifying the atom data.
🚦 Testing
How did you test these changes?
☑️ Checklist
build_docs
label