Skip to content
Open
Show file tree
Hide file tree
Changes from 15 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
94 changes: 94 additions & 0 deletions omc3/kmod_importer.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,10 @@ def import_kmod_results(opt: DotDict) -> None:
averaged_results[ip][opt.beam], # bpm results of the specific beam
)
]

import_kmod_summary_table(
meas_paths=opt.meas_paths, averaged_meas_paths=averaged_results, output_dir=opt.output_dir)

import_kmod_data(
model=df_model,
measurements=results_list,
Expand Down Expand Up @@ -301,6 +305,96 @@ def _get_betastar(df_model: tfs.TfsDataFrame, ip: str) -> list[float, float]:
# return [round(bstar, 3) for bstar in df_model.loc[ip, [f"{BETA}X", f"{BETA}Y"]]]
return df_model.loc[ip, [f"{BETA}X", f"{BETA}Y"]].tolist()

def import_kmod_summary_table(
Copy link
Member

Choose a reason for hiding this comment

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

This writes out things so the naming feels off. Maybe something like output_kmod_summary_tables?

Copy link
Member

Choose a reason for hiding this comment

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

I realize import_kmod_data suffers the same problem. We should look to fix that as well.

Copy link
Member

@JoschD JoschD Nov 27, 2025

Choose a reason for hiding this comment

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

(removed - updated below)

Copy link
Member

Choose a reason for hiding this comment

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

import_kmod_data makes a bit more sense, because it is importing it into the omc3 analysis folder in the same format as the other files there. Whereas this function really only outputs some text files, which actually do not really need to go into the omc3 results folder. But if there is a good naming suggestion, I am not strictly against renaming the import-data function as well.

meas_paths: Sequence[Path | str],
averaged_meas_paths: Sequence[tfs.TfsDataFrame],
Copy link
Member

Choose a reason for hiding this comment

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

Above at function call this is passed as averaged_results which is definitely not a Sequence[tfs.TfsDataFrame].

output_dir: Path | str = None
) -> None:

"""
Write the KMOD summary table from each results.tfs file.
It writes down the following files for each beam:
{beam}_kmod_sum_X.tfs: BETSTARX, ERRBETSTARX, BETWAISTX, ERRBETWAISTX, WAISTX, ERRWAISTX.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{beam}_kmod_sum_X.tfs: BETSTARX, ERRBETSTARX, BETWAISTX, ERRBETWAISTX, WAISTX, ERRWAISTX.
B{beam}_kmod_sum_X.tfs: BETSTARX, ERRBETSTARX, BETWAISTX, ERRBETWAISTX, WAISTX, ERRWAISTX.

Copy link
Member

Choose a reason for hiding this comment

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

none of the filenames here are correct actually anyway. There is no X/Y it's all in one file.
sum is actually summary and tables is summary_logbook. I prefer tables though.
And make them all lower-case.

{beam}_kmod_sum_X.tfs: BETSTARY, ERRBETSTARY, BETWAISTY, ERRBETWAISTY, WAISTY, ERRWAISTY.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{beam}_kmod_sum_X.tfs: BETSTARY, ERRBETSTARY, BETWAISTY, ERRBETWAISTY, WAISTY, ERRWAISTY.
B{beam}_kmod_sum_Y.tfs: BETSTARY, ERRBETSTARY, BETWAISTY, ERRBETWAISTY, WAISTY, ERRWAISTY.

{beam}_tables.txt to be quickly copy/pasted in the logbook.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{beam}_tables.txt to be quickly copy/pasted in the logbook.
B{beam}_tables.txt to be quickly copy/pasted in the logbook.

"""
Copy link
Member

@fsoubelet fsoubelet Nov 27, 2025

Choose a reason for hiding this comment

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

Overall there is not parameters documented in the docstring.


cols_x = ["BETSTARX", "ERRBETSTARX", "BETWAISTX", "ERRBETWAISTX", "WAISTX", "ERRWAISTX"]
cols_y = ["BETSTARY", "ERRBETSTARY", "BETWAISTY", "ERRBETWAISTY", "WAISTY", "ERRWAISTY"]
Comment on lines +322 to +323
Copy link
Member

Choose a reason for hiding this comment

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

I'll mark it here, but this is a general comment for the whole function: Use the constants for the columns. Check the other kmod-related files for how it is done there.

beams = [f"{BEAM_DIR}{1}", f"{BEAM_DIR}{2}"]

def output_table_single_beam(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it makes sense to have this as a nested function.

data_sngl_file_raw: 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.

Suggested change
data_sngl_file_raw: Path | str = None,
data_sngl_file_raw: Path | str | None = None,

Copy link
Member

Choose a reason for hiding this comment

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

Actually this should be

Suggested change
data_sngl_file_raw: Path | str = None,
data_sngl_file_raw: Path | None = None,

(see comment about line 336)

av_res_flag: bool = False,
Comment on lines +327 to +328
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of shortening names, it doesn't really help anyone.

) -> tuple[tfs.TfsDataFrame, tfs.TfsDataFrame]:

"""
It writes the single row for each kmod/results.tfs file.
"""
Comment on lines +331 to +333
Copy link
Member

Choose a reason for hiding this comment

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

This is quite unclear. Also need to document parameters, raises, returns


if not av_res_flag:
file_name = data_sngl_file_raw.parent.parent.name
Copy link
Member

Choose a reason for hiding this comment

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

This is not going to work is data_sngl_file_raw is a str, which the type hint says is accepted.

data_sngl_file = tfs.read(data_sngl_file_raw)
try:
label = data_sngl_file["LABEL"].iloc[0]
Copy link
Member

Choose a reason for hiding this comment

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

This could use the LABEL constant from omc3.optics_measurements.constants

second_magnet = label.split("-")[1]
relevant = second_magnet.split(".")[-1]
ip_number = relevant[1:]
ip_name = f"IP{ip_number}" # --- it extracts the IP from the magnet label, check for typos
Comment on lines +336 to +343
Copy link
Member

Choose a reason for hiding this comment

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

This is very hardcoded which makes me want a line or two of comments just above to explain the logic at hand here.

We should not expect the next dev to go through measurements folders and test files to understand this block.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking aloud: we take the singular entry in the LABEL column from results.tfs, split the expected structure to extract the (second) magnet, then the numbering (for example 1L1) and then the IP number itself.

Since we expect a hardcoded structure anyway, I would prefer that we document in the comment above that "the LABEL entry is in the form (i.e.) RQX.1R1-RQX.1L1 and the want to extract the IP number. The easiest way is to take the last number.

This would free us to just take df["LABEL"].iloc[0][-1]. Though I do see that a stacktrace with your code would point in a finer detail where the parsing goes wrong vs the expected structure.

except KeyError as e:
Comment on lines +338 to +344
Copy link
Member

Choose a reason for hiding this comment

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

Keep the try-part as short as possible (usually only one line), to make sure the Key error originates where you expect it to come from.

LOG.debug("Could not parse IP from LABEL. Skipping entry.", exc_info=e)
return None

df = data_sngl_file[cols_x + cols_y].iloc[[0]]
df.insert(0, "NAME", file_name)
df.insert(0, "IP", ip_name)
return df
Copy link
Member

Choose a reason for hiding this comment

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

On this code branch we return df, just below we return df_x, df_y. This clashes with the return type hint of the function.

Might this be better as 2 different functions (for averaged case vs individual case) using the same helper?


df_x = data_sngl_file_raw[cols_x].iloc[[0]]
df_y = data_sngl_file_raw[cols_y].iloc[[0]]
df_x.insert(0, "IP", data_sngl_file_raw['NAME'])
df_y.insert(0, "IP", data_sngl_file_raw['NAME'])
return df_x, df_y

grouped = {beam: [] for beam in beams}
for elem in meas_paths:
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer representative variable names. This elem is a path

LOG.debug(f"Reading results: {elem}.")
for beam in beams:
file_path = elem / beam / "results.tfs"
df = output_table_single_beam(file_path)
grouped[beam].append(df)
Comment on lines +359 to +365
Copy link
Member

Choose a reason for hiding this comment

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

Could use the comment before this block. Gathers for each beam the various Kmod result.tfs dataframes and stores in grouped. Is that correct?


all_av_x, all_av_y = [], []
Copy link
Member

@fsoubelet fsoubelet Nov 27, 2025

Choose a reason for hiding this comment

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

Suggested change
all_av_x, all_av_y = [], []
averaged_dfs_x, averaged_dfs_y = [], []

then propagate

for key, value in averaged_meas_paths.items():
LOG.debug(f"Reading averaged results: {key}")
df_av_x, df_av_y = output_table_single_beam(value[0], av_res_flag=True)
all_av_x.append(df_av_x)
all_av_y.append(df_av_y)
Comment on lines +367 to +372
Copy link
Member

Choose a reason for hiding this comment

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

Same here, could use a comment. Gather the kmod results averaged just earlier by the importer for each plane?

Comment on lines +367 to +372
Copy link
Member

@JoschD JoschD Nov 27, 2025

Choose a reason for hiding this comment

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

rename key and value to what the variables actually mean. (e.g. I have no idea what value[0] is supposed to be.


averaged_tables = {"X": (tfs.concat(all_av_x, ignore_index=True)).to_string(),
"Y": (tfs.concat(all_av_y, ignore_index=True)).to_string()}

for beam in beams:
LOG.debug(f"Processing result for: {beam}")
if grouped[beam]:
big_df = tfs.concat(grouped[beam], ignore_index=True)
big_df_x = big_df[["IP", "NAME"] + cols_x]
big_df_y = big_df[["IP", "NAME"] + cols_y]
txt_output = []
for name, df in [("X", big_df_x), ("Y", big_df_y)]:
table_str = df.to_string()
txt_output.append(f"\n============================================ {beam} Results ({name}-plane) ====================================================\n\n{table_str}\n")
txt_output.append(f"\n=========================== {beam} Averaged Results ({name}-plane) ============================================================\n\n{averaged_tables[name]}\n")

LOG.debug(f"Saving results for: {beam} ")
summary_path = output_dir / f"{beam}_summary_logbook.txt"
with summary_path.open("w") as f:
f.write("\n".join(txt_output))
tfs.write(output_dir/f"{beam}_kmod_summary.tfs", big_df)

LOG.info('KMOD summary outputs correctly saved.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LOG.info('KMOD summary outputs correctly saved.')
LOG.info(f"KMOD summary outputs saved at {output_dir/f'{beam}_kmod_summary.tfs'}")

even better is the full output path is stored in a variable before


return

# Script Mode ------------------------------------------------------------------

Expand Down
7 changes: 4 additions & 3 deletions tests/unit/test_kmod_importer.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from pathlib import Path

import pytest
import tfs

Expand Down Expand Up @@ -51,11 +50,12 @@ def test_full_kmod_import(tmp_path: Path, beam: int, ips: str):

# OUTPUT CHECKS --------------------------------------------
# Check the basics, if anything looks weird ---
assert len(list(tmp_path.glob(f"*{EXT}"))) == 4 # beta_kmod x/y, betastar x/y
assert len(list(tmp_path.glob(f"*{EXT}"))) == 6 # beta_kmod x/y, betastar x/y B1/B2_kmod_sum
assert len(list(tmp_path.glob("*.txt"))) == 2 # B1/B2_tables.txt
average_dir = tmp_path / AVERAGE_DIR

assert average_dir.is_dir()
assert len(list(average_dir.glob("*.pdf"))) == 3 * len(ips) # beta, beat and waist per IP
assert len(list(average_dir.glob("*.pdf"))) == 3 * len(ips) # beta, beat and waist per IP
assert len(list(average_dir.glob(f"*{EXT}"))) == 3 * len(ips) + N_EFFECTIVE_FILES[len(ips)] # AV_BPM: N_BEAM*N_IP, AV_BETASTAR: N_IPs, Effective: see map

# Check the content ---
Expand Down Expand Up @@ -92,3 +92,4 @@ def test_full_kmod_import(tmp_path: Path, beam: int, ips: str):
lumi_filename = _get_lumi_filename(betas, ip_a=ips[0], ip_b=ips[1])
assert (average_dir / lumi_filename).exists()


Copy link
Member

Choose a reason for hiding this comment

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

No need for this line

Copy link
Member

Choose a reason for hiding this comment

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

Could we add a little check that the written files are as expected?

Loading