Pytest Warnings Fix - 2 #2833

KasukabeDefenceForce · 2024-09-30T13:58:14Z

📝 Description

This PR aims at removing the warnings shown by pytest in the logs. It is continuation of PR #2823 .

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

tardis-bot · 2024-09-30T13:59:52Z

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

5	RET505	[ ] Unnecessary `elif` after `return` statement
4	G004  	[ ] Logging statement uses f-string
3	F401  	[*] `numpy` imported but unused
1	PIE808	[*] Unnecessary `start` argument in `range`
1	I001  	[*] Import block is un-sorted or un-formatted

Complete output(might be large):

tardis/energy_input/util.py:387:5: RET505 Unnecessary `elif` after `return` statement
tardis/io/model/parse_radiation_field_configuration.py:118:13: G004 Logging statement uses f-string
tardis/model/matter/composition.py:206:9: RET505 Unnecessary `else` after `return` statement
tardis/plasma/properties/atomic.py:554:9: RET505 Unnecessary `else` after `return` statement
tardis/plasma/properties/atomic.py:585:9: RET505 Unnecessary `else` after `return` statement
tardis/plasma/properties/atomic.py:598:17: G004 Logging statement uses f-string
tardis/plasma/properties/atomic.py:639:9: RET505 Unnecessary `else` after `return` statement
tardis/plasma/properties/nlte_rate_equation_solver.py:302:25: G004 Logging statement uses f-string
tardis/plasma/properties/nlte_rate_equation_solver.py:309:25: G004 Logging statement uses f-string
tardis/visualization/widgets/shell_info.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/visualization/widgets/shell_info.py:1:25: F401 [*] `tardis.base.run_tardis` imported but unused
tardis/visualization/widgets/shell_info.py:2:51: F401 [*] `tardis.io.atom_data.atom_web_download.download_atom_data` imported but unused
tardis/visualization/widgets/shell_info.py:12:17: F401 [*] `numpy` imported but unused
tardis/visualization/widgets/shell_info.py:299:25: PIE808 [*] Unnecessary `start` argument in `range`
Found 14 errors.
[*] 5 fixable with the `--fix` option.

tardis/plasma/properties/atomic.py

codecov · 2024-09-30T14:17:24Z

Codecov Report

Attention: Patch coverage is 76.92308% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.49%. Comparing base (b3af245) to head (e7d152c).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
tardis/energy_input/util.py	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2833      +/-   ##
==========================================
- Coverage   70.87%   70.49%   -0.39%     
==========================================
  Files         209      209              
  Lines       15580    15586       +6     
==========================================
- Hits        11043    10987      -56     
- Misses       4537     4599      +62

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

tardis-bot · 2024-10-01T14:46:48Z

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

Significantly changed benchmarks:

All benchmarks:

Benchmarks that have stayed the same:

| Change   | Before [56bc56be] <master>   | After [e7d152ca]    | Ratio   | Benchmark (Parameter)                                                                                                               |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
|          | 41.0±20μs                    | 53.6±10μs           | ~1.31   | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission                                         |
|          | 6.12±0.8μs                   | 6.87±1μs            | ~1.12   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket                                           |
|          | 3.89±0.04ms                  | 4.32±0.08ms         | ~1.11   | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom')                                   |
|          | 491±200ns                    | 541±200ns           | ~1.10   | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation                                      |
|          | 3.39±0.3μs                   | 2.90±0.3μs          | ~0.85   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket                                       |
|          | 712±90ns                     | 561±100ns           | ~0.79   | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation                              |
|          | 3.38±0.4μs                   | 3.57±0.5μs          | 1.06    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell                              |
|          | 19.5±4μs                     | 20.5±5μs            | 1.05    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
|          | 1.93±1μs                     | 1.98±1μs            | 1.02    | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators       |
|          | 203±0.1ns                    | 206±0.5ns           | 1.01    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body                                       |
|          | 1.18±0.01μs                  | 1.19±0μs            | 1.01    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary                |
|          | 2.72±0.4ms                   | 2.75±0.6ms          | 1.01    | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop                        |
|          | 2.87±0ms                     | 2.87±0.06ms         | 1.00    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter')                                     |
|          | 37.9±0.02s                   | 37.8±0.04s          | 1.00    | run_tardis.BenchmarkRunTardis.time_run_tardis                                                                                       |
|          | 1.04±0m                      | 1.04±0m             | 1.00    | run_tardis.BenchmarkRunTardis.time_run_tardis_rpacket_tracking                                                                      |
|          | 2.08±0m                      | 2.08±0m             | 1.00    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_FormalIntegrator_functions                                 |
|          | 1.67±0.01ms                  | 1.67±0ms            | 1.00    | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop                             |
|          | 31.0±0.03μs                  | 30.9±0.01μs         | 1.00    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_tracker_list                  |
|          | 1.42±0.4μs                   | 1.40±0.4μs          | 0.99    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line                    |
|          | 63.6±0.2ms                   | 63.0±0.2ms          | 0.99    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe                  |
|          | 7.90±2μs                     | 7.73±2μs            | 0.98    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley                                    |
|          | 724±1ns                      | 705±0.6ns           | 0.97    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter                                       |
|          | 561±200ns                    | 531±200ns           | 0.95    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation                                |
|          | 49.5±20μs                    | 47.0±20μs           | 0.95    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter                                          |

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

pyproject.toml

tardis/plasma/properties/atomic.py

andrewfullard · 2024-10-07T16:50:01Z

tardis/io/model/parse_radiation_field_configuration.py

    )
+    value = np.clip(value, 0, None)


What warning does this resolve?

RuntimeWarning: invalid value encountered in sqrt
- np.sqrt(

tardis/plasma/properties/atomic.py

andrewfullard · 2024-10-07T16:51:01Z

tardis/plasma/properties/nlte_rate_equation_solver.py

@@ -937,7 +935,7 @@ def ion_matrix(ion_coefficients, atomic_number, ion_number):
    offdiag = np.zeros(atomic_number)
    index = ion_coefficients.index
    for i in index:
-        offdiag[i] = ion_coefficients.loc[i]
+        offdiag[i] = float(ion_coefficients.loc[i].iloc[0])


Does it need to have nested .loc.iloc?

Yep, to make sure it only extracts a single value and not a series. I will replace iloc with values for consistency and it is slightly faster than iloc.

andrewfullard · 2024-10-07T16:51:19Z

tardis/plasma/properties/nlte_rate_equation_solver.py

@@ -961,7 +959,7 @@ def recomb_matrix(recomb_coefficients, atomic_number, ion_number):
    offdiag = np.zeros(atomic_number)
    index = recomb_coefficients.index
    for i in index:
-        offdiag[i] = recomb_coefficients.loc[i]
+        offdiag[i] = float(recomb_coefficients.loc[i].values[0])


Why does this use .loc.values?

it does the same thing as iloc, I will update the previous function with values instead of iloc

jvshields · 2024-10-10T14:12:04Z

tardis/io/tests/test_atomic.py

@@ -48,6 +48,7 @@ def test_atom_data_levels(levels):


 def test_atom_data_lines(lines):
+    lines = lines.sort_index()


I think we don't want to overwrite or change pytest fixtures like this. We ran into problems awhile back with tests passing when they were run in a specific order, but not otherwise because of changing the fixtures in tests.

Probably just save the sorted lines to a new variable?

jvshields · 2024-10-10T14:15:40Z

tardis/plasma/properties/nlte_rate_equation_solver.py

@@ -935,7 +933,7 @@ def ion_matrix(ion_coefficients, atomic_number, ion_number):
    offdiag = np.zeros(atomic_number)
    index = ion_coefficients.index
    for i in index:
-        offdiag[i] = ion_coefficients.loc[i]
+        offdiag[i] = float(ion_coefficients.loc[i].values[0])


I'm somewhat surprised you have to cast this to a float. Are ion coefficients ever not floats? Or is this something to do with trying to obtain the value instead of getting a pandas series?

I ran it again and it showed no warnings without float so I think its fine without it.

tardis-bot · 2024-10-14T14:44:14Z

*beep* *bop*

Hi, human.

The docs workflow has succeeded ✔️

Click here to see your results.

KasukabeDefenceForce marked this pull request as ready for review September 30, 2024 14:05

andrewfullard reviewed Sep 30, 2024

View reviewed changes

tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved

andrewfullard added the codestyle label Oct 1, 2024

KasukabeDefenceForce force-pushed the more_warnings branch 4 times, most recently from d8b8b08 to bc2c160 Compare October 3, 2024 14:02

KasukabeDefenceForce marked this pull request as draft October 3, 2024 14:33

KasukabeDefenceForce force-pushed the more_warnings branch 2 times, most recently from 71e7f0a to b52bece Compare October 4, 2024 10:21

KasukabeDefenceForce marked this pull request as ready for review October 4, 2024 12:08

atharva-2001 reviewed Oct 7, 2024

View reviewed changes

pyproject.toml Show resolved Hide resolved

jvshields reviewed Oct 7, 2024

View reviewed changes

tardis/plasma/properties/atomic.py Outdated Show resolved Hide resolved

Knights-Templars previously approved these changes Oct 7, 2024

View reviewed changes

andrewfullard requested review from andrewfullard, jvshields and atharva-2001 October 7, 2024 16:49

andrewfullard reviewed Oct 7, 2024

View reviewed changes

KasukabeDefenceForce dismissed Knights-Templars’s stale review via 2ce2a49 October 10, 2024 09:33

KasukabeDefenceForce added 7 commits October 10, 2024 15:06

Remove unrecongnised attribute

dab6cd7

Replace applymap with map function

0fa623a

Remove pytest warnings

1310637

using np.exp to handle runtime warning.

ddfe7fc

Run black formatting on files

ebaf57d

replace extra condition with mask

994018f

Replacing iloc with values

2758214

KasukabeDefenceForce force-pushed the more_warnings branch from 2ce2a49 to 2758214 Compare October 10, 2024 09:36

jvshields reviewed Oct 10, 2024

View reviewed changes

KasukabeDefenceForce added 2 commits October 11, 2024 19:23

assign sorted lines to a new variable

70d81fd

remove explicit float conversion

e7d152c

andrewfullard added the build-docs label Oct 14, 2024

andrewfullard approved these changes Oct 14, 2024

View reviewed changes

jvshields approved these changes Oct 14, 2024

View reviewed changes

andrewfullard merged commit 47d64bf into tardis-sn:master Oct 14, 2024
26 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pytest Warnings Fix - 2 #2833

Pytest Warnings Fix - 2 #2833

KasukabeDefenceForce commented Sep 30, 2024

tardis-bot commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

tardis-bot commented Oct 1, 2024 •

edited

Loading

andrewfullard Oct 7, 2024

KasukabeDefenceForce Oct 10, 2024

andrewfullard Oct 7, 2024

KasukabeDefenceForce Oct 10, 2024

andrewfullard Oct 7, 2024

KasukabeDefenceForce Oct 10, 2024

jvshields Oct 10, 2024

KasukabeDefenceForce Oct 11, 2024

jvshields Oct 10, 2024

KasukabeDefenceForce Oct 14, 2024

tardis-bot commented Oct 14, 2024

		@@ -48,6 +48,7 @@ def test_atom_data_levels(levels):


		def test_atom_data_lines(lines):
		lines = lines.sort_index()

Pytest Warnings Fix - 2 #2833

Pytest Warnings Fix - 2 #2833

Conversation

KasukabeDefenceForce commented Sep 30, 2024

📝 Description

📌 Resources

🚦 Testing

☑️ Checklist

tardis-bot commented Sep 30, 2024 • edited Loading

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

tardis-bot commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tardis-bot commented Oct 14, 2024

tardis-bot commented Sep 30, 2024 •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

tardis-bot commented Oct 1, 2024 •

edited

Loading