Skip to content

Reintroducing Site reports#2166

Merged
EshitaJoshi merged 16 commits into
mainfrom
obs-reports
May 7, 2026
Merged

Reintroducing Site reports#2166
EshitaJoshi merged 16 commits into
mainfrom
obs-reports

Conversation

@EshitaJoshi
Copy link
Copy Markdown
Collaborator

@EshitaJoshi EshitaJoshi commented May 5, 2026

@EshitaJoshi EshitaJoshi added no-changelog-needed No Changelog entry required for this PR and removed no-changelog-needed No Changelog entry required for this PR labels May 6, 2026
@EshitaJoshi EshitaJoshi marked this pull request as ready for review May 6, 2026 09:55
@EshitaJoshi EshitaJoshi requested a review from GernotMaier May 6, 2026 12:45
Copy link
Copy Markdown
Contributor

@GernotMaier GernotMaier left a comment

Choose a reason for hiding this comment

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

Thanks for this - two minor requests.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We do not want a lot of functional code in the application scripts. Can you move those new functions into a module?

It is probably either src/simtools/visualization/plot_array_layout.py or src/simtools/layout/array_layout_utils.py.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done, moved to src/simtools/visualization/plot_array_layout.py

file.write("\n")
version = self.model_version.replace(".", "-")
filename = f"OBS-{self.site}_{layout_name}_{version}.png"
version = self.model_version[:3]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remind me: did we decide to plot only major.minor and not patch versions? This is probably the only place where this is the case.

Comment thread src/simtools/simtel/simtel_table_reader.py
@ctao-sonarqube
Copy link
Copy Markdown

ctao-sonarqube Bot commented May 7, 2026

@EshitaJoshi
Copy link
Copy Markdown
Collaborator Author

@GernotMaier Can you have another look at this please and let me know if it's ok to merge?

Copy link
Copy Markdown
Contributor

@GernotMaier GernotMaier left a comment

Choose a reason for hiding this comment

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

Thanks - looks good.

Someone else might not be able to sleep because of the mix of underscore and hyphens (e.g. in array_layout_subsystem_msts_North_ground_6-0-2.png). I don't mind.

@EshitaJoshi
Copy link
Copy Markdown
Collaborator Author

Someone else might not be able to sleep because of the mix of underscore and hyphens (e.g. in array_layout_subsystem_msts_North_ground_6-0-2.png). I don't mind.

The issue is that when we have a filename ending with major.minor.patch version, the code seems to interpret the .patch as the existing file suffix and replaces it with .png so using dashes felt simpler, but we can change it at any point if someone complains

@EshitaJoshi EshitaJoshi merged commit 9ef0cbb into main May 7, 2026
18 checks passed
@EshitaJoshi EshitaJoshi deleted the obs-reports branch May 7, 2026 13:26
@GernotMaier
Copy link
Copy Markdown
Contributor

Someone else might not be able to sleep because of the mix of underscore and hyphens (e.g. in array_layout_subsystem_msts_North_ground_6-0-2.png). I don't mind.

The issue is that when we have a filename ending with major.minor.patch version, the code seems to interpret the .patch as the existing file suffix and replaces it with .png so using dashes felt simpler, but we can change it at any point if someone complains

Yes - but you could use consistently underscores rray_layout_subsystem_msts_North_ground_6_0_2.png

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants