Moves non-physical input check outsode I/O module #2923

Sonu0305 · 2025-01-04T19:39:11Z

📝 Description

Type: 🎢 infrastructure

Created a module named physics for storing the radiation_field.py (validation) separately outside I/O module and modified necessary few lines of parse_radiation_field_configuration.py

Closes #2681

📌 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 · 2025-01-04T19:41:06Z

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

2	I001  	[*] Import block is un-sorted or un-formatted
1	G004  	[ ] Logging statement uses f-string
1	INP001	[ ] File `tardis/radiation_field/validate_radiation_field.py` is part of an implicit namespace package. Add an `__init__.py`.
1	E999  	[ ] SyntaxError: Expected an expression
1	W292  	[*] No newline at end of file

Complete output(might be large):

.mailmap:1:38: E999 SyntaxError: Expected an expression
.mailmap:292:39: W292 [*] No newline at end of file
tardis/io/model/parse_radiation_field_configuration.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/radiation_field/validate_radiation_field.py:1:1: INP001 File `tardis/radiation_field/validate_radiation_field.py` is part of an implicit namespace package. Add an `__init__.py`.
tardis/radiation_field/validate_radiation_field.py:1:1: I001 [*] Import block is un-sorted or un-formatted
tardis/radiation_field/validate_radiation_field.py:25:13: G004 Logging statement uses f-string
Found 6 errors.
[*] 3 fixable with the `--fix` option.

tardis-bot · 2025-01-04T20:38:47Z

*beep* *bop*
Hi human,
I ran benchmarks as you asked comparing master (7e37d5f) and the latest commit (0fdc3fb).
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 [7e37d5fe] <master>   | After [0fdc3fbf]    | Ratio   | Benchmark (Parameter)                                                                                                               |
|----------|------------------------------|---------------------|---------|-------------------------------------------------------------------------------------------------------------------------------------|
|          | 1.32±0.3μs                   | 1.65±0.4μs          | ~1.25   | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_line                    |
|          | 531±200ns                    | 652±100ns           | ~1.23   | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_photoabsorption_opacity_calculation                              |
|          | 3.44±0.8μs                   | 3.09±0.3μs          | ~0.90   | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_bad_vpacket                                       |
|          | 2.16±2μs                     | 1.88±2μs            | ~0.87   | transport_montecarlo_estimators_radfield_estimator_calcs.BenchmarkMontecarloMontecarloNumbaPacket.time_update_line_estimators       |
|          | 45.9±20μs                    | 50.4±20μs           | 1.10    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_emission                                         |
|          | 2.56±0.5ms                   | 2.71±0.5ms          | 1.06    | transport_montecarlo_single_packet_loop.BenchmarkTransportMontecarloSinglePacketLoop.time_single_packet_loop                        |
|          | 571±200ns                    | 601±300ns           | 1.05    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_pair_creation_opacity_calculation                                |
|          | 1.17±0μs                     | 1.22±0μs            | 1.04    | transport_geometry_calculate_distances.BenchmarkTransportGeometryCalculateDistances.time_calculate_distance_boundary                |
|          | 6.03±0.9μs                   | 6.29±0.7μs          | 1.04    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket                                           |
|          | 1.65±0.1ms                   | 1.68±0ms            | 1.02    | transport_montecarlo_main_loop.BenchmarkTransportMontecarloMontecarloMainLoop.time_montecarlo_main_loop                             |
|          | 64.6±0.2ms                   | 65.1±1ms            | 1.01    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_rpacket_trackers_to_dataframe                  |
|          | 3.42±0.4μs                   | 3.46±0.5μs          | 1.01    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_within_shell                              |
|          | 2.83±0.02ms                  | 2.84±0.03ms         | 1.00    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('scatter')                                     |
|          | 1.05±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                                 |
|          | 48.9±20μs                    | 49.0±30μs           | 1.00    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_line_scatter                                          |
|          | 3.76±0.02ms                  | 3.70±0.02ms         | 0.99    | opacities_opacity_state.BenchmarkOpacitiesOpacityState.time_opacity_state_initialize('macroatom')                                   |
|          | 38.6±0.06s                   | 38.2±0.08s          | 0.99    | run_tardis.BenchmarkRunTardis.time_run_tardis                                                                                       |
|          | 20.2±5μs                     | 20.1±5μs            | 0.99    | transport_montecarlo_packet_trackers.BenchmarkTransportMontecarloPacketTrackers.time_generate_rpacket_last_interaction_tracker_list |
|          | 7.36±2μs                     | 7.29±1μs            | 0.99    | transport_montecarlo_vpacket.BenchmarkMontecarloMontecarloNumbaVpacket.time_trace_vpacket_volley                                    |
|          | 541±100ns                    | 531±200ns           | 0.98    | opacities_opacity.BenchmarkMontecarloMontecarloNumbaOpacities.time_compton_opacity_calculation                                      |
|          | 740±0.8ns                    | 726±2ns             | 0.98    | transport_montecarlo_interaction.BenchmarkTransportMontecarloInteraction.time_thomson_scatter                                       |
|          | 208±0.01ns                   | 203±0.1ns           | 0.97    | spectrum_formal_integral.BenchmarkTransportMontecarloFormalIntegral.time_intensity_black_body                                       |
|          | 38.9±0.03μs                  | 37.8±0.1μs          | 0.97    | 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

Sonu0305 · 2025-01-05T01:28:04Z

Hi @wkerzendorf @Knights-Templars @atharva-2001 @andrewfullard @epassaro ,
Could one of you please review this when you get a chance?
Thank You for Your Time and Support.

andrewfullard · 2025-01-06T15:27:48Z

pyproject.toml

@@ -190,5 +190,5 @@ owner = "tardis-sn"
 repo = "tardis"

 [tool.codespell]
-skip = "*.png,*.ggb,*.jpg,*.gif,*.ico,docs/contributing/CHANGELOG.md,docs/tardis.bib"
+skip = "*.ipynb,*.png,*.ggb,*.jpg,*.gif,*.ico,docs/contributing/CHANGELOG.md,docs/tardis.bib,docs/resources/research_done_using_TARDIS/research_papers.rst"


Why is this change included?

To avoid codespell job failure like it is mentioned in #2908 but it is not merged yet, hence included in this.

A PR should ideally have only one purpose, and this will lead to merge conflicts that must be resolved once #2908 is merged

Sorry, I got your point, will make sure this does not repeat.

andrewfullard · 2025-01-06T15:28:09Z

tardis/physics/radiation_field.py

Why should validation be in a directory called "physics"?

I understood the title of #2681, 'Move ... to a physics module,' as meaning to move it to a physics directory (module). Sorry if I misunderstood.

I see. The term was meant generically, because most of TARDIS consists of modules that calculate physics. Thus these checks should be placed in the most relevant pre-existing module.

Okay understood, I'll make the necessary changes.

Sonu0305 · 2025-01-07T14:43:13Z

@andrewfullard
Done, made the necessary changes:
I deleted the new physics directory that i created, changed the new file name from
radiation_field.py -> test_radiation_field.py
and moved it to tardis\radiation_field directory
Thank You

This reverts commit aa7a3ed.

This reverts commit 1a1d8b9.

Sonu0305 · 2025-01-07T16:21:58Z

Also @andrewfullard ,
To ensure that there are no merge conflicts when #2908 is merged, I reverted the changes (excluded the changes of codespell failure over here) to ensure clean merging.
Sorry for any inconvenience, will take care of it next time.
Please review the changes when you have a chance, Thank You.

andrewfullard · 2025-01-07T21:32:13Z

tardis/radiation_field/test_radiation_field.py

I think "validate_radiation_field" would be a better name

Sonu0305 · 2025-01-08T03:46:05Z

@andrewfullard ,
Done, Thank You.

Sonu0305 · 2025-01-08T18:46:48Z

Hi @andrewfullard @jvshields ,
I wanted to confirm if there are any additional modifications required from my side, as I don't believe the test failures are related to this PR, please let me know when you have a chance.
Thank You.

Sonu0305 added 3 commits January 5, 2025 01:00

moves non-physical input check

9716029

fixes codespell failure as in tardis-sn#2908

1a1d8b9

updated mailmap

d6609b3

fixes codespell failure as in tardis-sn#2908

aa7a3ed

andrewfullard added the restructure label Jan 6, 2025

andrewfullard reviewed Jan 6, 2025

View reviewed changes

Sonu0305 added 2 commits January 7, 2025 01:30

Merge branch 'tardis-sn:master' into fix#2681

cab6bd6

made necessary changes to path

3fd4ab7

Sonu0305 added 2 commits January 7, 2025 21:42

Revert "fixes codespell failure as in tardis-sn#2908"

7a77bff

This reverts commit aa7a3ed.

Revert "fixes codespell failure as in tardis-sn#2908"

30560cc

This reverts commit 1a1d8b9.

andrewfullard reviewed Jan 7, 2025

View reviewed changes

Sonu0305 added 2 commits January 8, 2025 08:37

Rename test_radiation_field.py to validate_radiation_field.py

d78865c

Update parse_radiation_field_configuration.py

0fdc3fb

andrewfullard approved these changes Jan 8, 2025

View reviewed changes

andrewfullard requested a review from jvshields January 8, 2025 17:06

jvshields approved these changes Jan 8, 2025

View reviewed changes

andrewfullard merged commit 4d9f61f into tardis-sn:master Jan 8, 2025
10 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moves non-physical input check outsode I/O module #2923

Moves non-physical input check outsode I/O module #2923

Sonu0305 commented Jan 4, 2025

tardis-bot commented Jan 4, 2025 •

edited

Loading

tardis-bot commented Jan 4, 2025 •

edited

Loading

Sonu0305 commented Jan 5, 2025

andrewfullard Jan 6, 2025

Sonu0305 Jan 6, 2025

andrewfullard Jan 7, 2025

Sonu0305 Jan 7, 2025

andrewfullard Jan 6, 2025

Sonu0305 Jan 6, 2025

andrewfullard Jan 7, 2025

Sonu0305 Jan 7, 2025

Sonu0305 commented Jan 7, 2025 •

edited

Loading

Sonu0305 commented Jan 7, 2025

andrewfullard Jan 7, 2025

Sonu0305 commented Jan 8, 2025

Sonu0305 commented Jan 8, 2025

Moves non-physical input check outsode I/O module #2923

Moves non-physical input check outsode I/O module #2923

Conversation

Sonu0305 commented Jan 4, 2025

📝 Description

📌 Resources

🚦 Testing

☑️ Checklist

tardis-bot commented Jan 4, 2025 • edited Loading

tardis-bot commented Jan 4, 2025 • edited Loading

Sonu0305 commented Jan 5, 2025

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

Sonu0305 commented Jan 7, 2025 • edited Loading

Sonu0305 commented Jan 7, 2025

Choose a reason for hiding this comment

Sonu0305 commented Jan 8, 2025

Sonu0305 commented Jan 8, 2025

tardis-bot commented Jan 4, 2025 •

edited

Loading

tardis-bot commented Jan 4, 2025 •

edited

Loading

Sonu0305 commented Jan 7, 2025 •

edited

Loading