Skip to content
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

Sum velocity-binned vpkt spectra across all ranks #284

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions artistools/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

vpackets_total_active_spectral_region = vpkt_txt.readline().split()
vpkt_config["vpackets total active spectral region"] = vpackets_total_active_spectral_region

optically_thick_cells = vpkt_txt.readline().split()
vpkt_config["optically thick cells"] = optically_thick_cells
Comment on lines +265 to +266
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


optical_depths = vpkt_txt.readline().split()
vpkt_config["optical depths"] = optical_depths

velocity_grid_map = vpkt_txt.readline().split()
vpkt_config["velocity grid map"] = velocity_grid_map

velocity_grid_map_active = vpkt_txt.readline().split()
vpkt_config["velocity grid map active"] = velocity_grid_map_active

velocity_grid_map_regions = vpkt_txt.readline().split()
vpkt_config["Number Of Velocity Maps"] = velocity_grid_map_regions[0]

vpkt_config["Velocity Map Regions"] = velocity_grid_map_regions[1:]

return vpkt_config


Expand Down
1 change: 1 addition & 0 deletions artistools/spectra/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from artistools.spectra.spectra import get_vspecpol_data as get_vspecpol_data
from artistools.spectra.spectra import get_vspecpol_spectrum as get_vspecpol_spectrum
from artistools.spectra.spectra import make_averaged_vspecfiles as make_averaged_vspecfiles
from artistools.spectra.spectra import make_virtual_grid_summed_file as make_virtual_grid_summed_file
from artistools.spectra.spectra import make_virtual_spectra_summed_file as make_virtual_spectra_summed_file
from artistools.spectra.spectra import print_integrated_flux as print_integrated_flux
from artistools.spectra.spectra import read_spec_res as read_spec_res
Expand Down
8 changes: 8 additions & 0 deletions artistools/spectra/plotspectra.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

# To get better statistics for polarisation use multiple runs of the same simulation. This will then average the
# files produced by makevspecpol for all simulations.
parser.add_argument(
Expand Down Expand Up @@ -1473,6 +1477,10 @@ def main(args: argparse.Namespace | None = None, argsraw: Sequence[str] | None =
atspectra.make_virtual_spectra_summed_file(args.specpath[0])
return

if args.makevspecgrid:
atspectra.make_virtual_grid_summed_file(args.specpath[0])
return

if args.averagevspecpolfiles:
atspectra.make_averaged_vspecfiles(args)
return
Expand Down
77 changes: 77 additions & 0 deletions artistools/spectra/spectra.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

print("Generating all-rank summed vspec Grid files..")
nprocs = get_nprocs(modelpath)
print("nprocs", nprocs)
vpktconfig = get_vpkt_config(modelpath)

nvirtual_spectra = int(vpktconfig["nobsdirections"])
nvmaps = int(vpktconfig["Number Of Velocity Maps"])

# 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

VGRID_NY = len(vpkt_grid_data[0].unique())
VGRID_NZ = len(vpkt_grid_data[1].unique())

print(f"VGRID_NY: {VGRID_NY}, VGRID_NZ: {VGRID_NZ}")

total_grid_points = VGRID_NY * VGRID_NZ # Total grid points in one layer
rows_per_obsdir = total_grid_points * nvmaps # Rows for one observer direction

# Output filename
output_filename_template = "Vpkt_grid_total_{}.txt"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# Initialize variables for summation
summed_data = None
velocity_columns = None # velocity separately

for filename in vpkt_grid_files:
data = np.loadtxt(filename)
print(f"Processing {filename}")

# Validate row count
expected_rows = rows_per_obsdir * nvirtual_spectra

if data.shape[0] != expected_rows:
print(f"Unexpected number of rows in {filename}. Expected {expected_rows}, got {data.shape[0]}.")
Comment on lines +595 to +596
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Separate velocity and other columns
if velocity_columns is None:
velocity_columns = data[:, :2] # Assuming the first two columns are velocity
else:
print(f"Velocity columns in {filename} do not match previous files.")
Comment on lines +601 to +602
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Sum the remaining columns
if summed_data is None:
summed_data = data[:, 2:] # Exclude velocity columns from summation
else:
summed_data += data[:, 2:]
print("Files successfully processed.")
# Combine velocity and summed data
valid_data = tuple(array for array in (velocity_columns, summed_data) if array is not None)
# Perform the horizontal stack
final_data = np.hstack(valid_data)

# 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.")
Comment on lines +615 to +619
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


# Split combined data by observer direction and write to output files
for obsdir in range(nvirtual_spectra):
start_idx = obsdir * rows_per_obsdir
end_idx = start_idx + rows_per_obsdir

# Extract data for this observer direction
obs_data = final_data[start_idx:end_idx, :]

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for row in obs_data:
f.write(", ".join(map(str, row)) + "\n")

print("Velocity Grid successfully Written.")


def make_averaged_vspecfiles(args: argparse.Namespace) -> None:
filenames = [
vspecfile.name
Expand Down
Loading