Sum velocity-binned vpkt spectra across all ranks #284

jpollin98 · 2024-12-20T15:12:59Z

Hi Luke, Is there something I've done wrong with this PR ? I'm not sure why it was reverted, as it passed all the tests?

lukeshingles · 2024-12-20T15:35:34Z

On holidays now, will give a review and suggestions within a couple of weeks.

codspeed-hq · 2024-12-20T15:54:35Z

CodSpeed Performance Report

Merging #284 will not alter performance

_{Comparing sum_all_polatisation_grids (0b6fdad) with main (093339c)}

Summary

✅ 24 untouched benchmarks

lukeshingles

Aside from the style suggestions, my main question is whether summing the output of each rank is really the correct thing to do versus averaging them.

lukeshingles · 2025-01-04T20:14:42Z

artistools/misc.py

@@ -258,6 +258,27 @@ def get_vpkt_config(modelpath: Path | str) -> dict[str, t.Any]:
            int(x) for x in vpkt_txt.readline().split()
        )

+        # read the next line


This comment doesn't add information since the call is to to readline().

lukeshingles · 2025-01-04T20:27:55Z

artistools/spectra/spectra.py

+
+    # Read the first file to get the dimensions
+    vpkt_grid_files = [f"vpkt_grid_{mpirank:04d}.out" for mpirank in range(nprocs)]
+    vpkt_grid_data = pd.read_csv(vpkt_grid_files[0], sep=" ", header=None)


I prefer that we use polars for all new code except where it doesn't provide the necessary functionality (e.g. no multi-character delimiters in pl.read_csv), but this is a minor point. Eventually, we will be able to drop the global pandas imports and save ~500ms of start up time, which will improve the command-line autocomplete responsiveness.

lukeshingles · 2025-01-04T22:24:37Z

artistools/spectra/spectra.py

+        else:
+            print(f"Velocity columns in {filename} do not match previous files.")


It looks like the condition is missing. After processing the first file, velocity_columns will be not None, so I would expect every additional file to print this warning.

lukeshingles · 2025-01-04T22:34:01Z

artistools/spectra/spectra.py

+    rows_per_obsdir = total_grid_points * nvmaps  # Rows for one observer direction
+
+    # Output filename
+    output_filename_template = "Vpkt_grid_total_{}.txt"


Please avoid using capital letters in filenames to match the project convention (and avoid issues with case-insensitive file systems).
Are you sure that it is correct to sum these spectra being summed instead of averaging them? For the normal vpkt spectra, the energy contributions are divided by the total number of ranks by artis, but this does not seem to be the case for the vpkt velocity grid. Should the division by num procs be done by artistools?

lukeshingles · 2025-01-04T22:37:26Z

artistools/spectra/spectra.py

@@ -558,6 +558,83 @@ def make_virtual_spectra_summed_file(modelpath: Path | str) -> None:
        print(f"Saved {outfile}")


+def make_virtual_grid_summed_file(modelpath: Path | str) -> None:


"virtual grid" is not very descriptive to me. How about something like make_averaged_vpkt_grid_files? (depending on the sum vs average question)

lukeshingles · 2025-01-04T22:42:48Z

artistools/spectra/spectra.py

+
+        # Write to output file
+        output_path = Path(output_filename_template.format(obsdir))
+        with output_path.open("w", encoding="utf-8") as f:  # Specify encoding


The "Specify encoding" comment does not add any information. Remove it unless there is a good reason to keep it.

lukeshingles · 2025-01-04T22:43:48Z

artistools/spectra/spectra.py

+        if data.shape[0] != expected_rows:
+            print(f"Unexpected number of rows in {filename}. Expected {expected_rows}, got {data.shape[0]}.")


Shouldn't this case cause a crash rather than just printing a warning?

lukeshingles · 2025-01-06T10:17:22Z

artistools/spectra/spectra.py

+    # Ensure final data has exactly 5 columns
+    if final_data.shape[1] > 5:
+        print("Data has more than 5 columns. Should only have N1, N2, I, Q, U.")
+    if final_data.shape[1] < 5:
+        print("Data has less than 5 columns. Should only have N1, N2, I, Q, U.")


Simpler than this would be:

if final_data.shape[1] != 5: msg = f"Data has {final_data.shape[1]} != 5 columns. Should only have N1, N2, I, Q, U." raise ValueError(msg)

lukeshingles · 2025-01-06T10:20:43Z

artistools/spectra/plotspectra.py

@@ -1341,6 +1341,10 @@ def addargs(parser) -> None:
        "--makevspecpol", action="store_true", help="Make file summing the virtual packet spectra from all ranks"
    )

+    parser.add_argument(
+        "--makevspecgrid", action="store_true", help="Make file summing the virtual packet grid from all ranks"


Consider renaming vspecgrid to vspecvelgrid to be more descriptive. I plan to do this in the artis code at some point. Actually, I didn't know that anyone was using the vspecgrid functionality. It might be worth adding some extra em_pos, em_time columns to the vpackets*.out files so that we can do similar plots directly from that data.

lukeshingles · 2025-01-06T10:26:30Z

artistools/misc.py

+        optically_thick_cells = vpkt_txt.readline().split()
+        vpkt_config["optically thick cells"] = optically_thick_cells


I guess you would have been looking at the artis source code to determine the meaning of items in the vpkt.txt file. Artis calls the items on this line override_thickcell_tau and cell_is_optically_thick_vpkt, which I find more description than "optically thick cells".

Please follow the existing convention and avoid line noise, e.g.,

vpkt_config["override_thickcell_tau"], vpkt["cell_is_optically_thick_vpkt"] = vpkt_txt.readline().split()

Similarly for the following lines, consider directly re-using the artis variable names for consistency, or coming up with a more descriptive name while avoiding spaces in the dictionary keys to match the existing code.

jpollin98 requested a review from lukeshingles as a code owner December 20, 2024 15:13

lukeshingles force-pushed the main branch 2 times, most recently from 83f3ebd to 965f0ac Compare December 23, 2024 21:18

lukeshingles changed the title ~~Sum all polatisation grids~~ Sum velocity-binned vpkt spectra from all ranks Jan 4, 2025

lukeshingles changed the title ~~Sum velocity-binned vpkt spectra from all ranks~~ Average velocity-binned vpkt spectra across all ranks Jan 4, 2025

lukeshingles changed the title ~~Average velocity-binned vpkt spectra across all ranks~~ Sum velocity-binned vpkt spectra across all ranks Jan 4, 2025

lukeshingles requested changes Jan 6, 2025

View reviewed changes

jpollin98 added 5 commits January 6, 2025 10:34

Added ability to sum Polarisation grids

4912a40

removed comments

c21b26c

Improved formatting

19843be

Update spectra.py

3ba5bc5

Fixed pre-commit issues and updated formatting

0b6fdad

lukeshingles force-pushed the sum_all_polatisation_grids branch from 379218c to 0b6fdad Compare January 6, 2025 10:35

lukeshingles temporarily deployed to test January 6, 2025 10:39 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sum velocity-binned vpkt spectra across all ranks #284

Sum velocity-binned vpkt spectra across all ranks #284

jpollin98 commented Dec 20, 2024 •

edited

Loading

lukeshingles commented Dec 20, 2024

codspeed-hq bot commented Dec 20, 2024 •

edited

Loading

lukeshingles left a comment •

edited

Loading

lukeshingles Jan 4, 2025

lukeshingles Jan 4, 2025

lukeshingles Jan 4, 2025

lukeshingles Jan 4, 2025

lukeshingles Jan 4, 2025

lukeshingles Jan 4, 2025

lukeshingles Jan 4, 2025

lukeshingles Jan 6, 2025

lukeshingles Jan 6, 2025

lukeshingles Jan 6, 2025

		else:
		print(f"Velocity columns in {filename} do not match previous files.")

		@@ -558,6 +558,83 @@ def make_virtual_spectra_summed_file(modelpath: Path \| str) -> None:
		print(f"Saved {outfile}")


		def make_virtual_grid_summed_file(modelpath: Path \| str) -> None:

		if data.shape[0] != expected_rows:
		print(f"Unexpected number of rows in {filename}. Expected {expected_rows}, got {data.shape[0]}.")

		optically_thick_cells = vpkt_txt.readline().split()
		vpkt_config["optically thick cells"] = optically_thick_cells

Sum velocity-binned vpkt spectra across all ranks #284

Are you sure you want to change the base?

Sum velocity-binned vpkt spectra across all ranks #284

Conversation

jpollin98 commented Dec 20, 2024 • edited Loading

lukeshingles commented Dec 20, 2024

codspeed-hq bot commented Dec 20, 2024 • edited Loading

Merging #284 will not alter performance

Summary

lukeshingles left a comment • 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

Choose a reason for hiding this comment

jpollin98 commented Dec 20, 2024 •

edited

Loading

codspeed-hq bot commented Dec 20, 2024 •

edited

Loading

lukeshingles left a comment •

edited

Loading