Skip to content

Add functionality to assign valid telescope numbers to additional MSTs in legacy prod6 simulations.#2171

Merged
GernotMaier merged 4 commits into
mainfrom
legacy-prod6-mst2
May 7, 2026
Merged

Add functionality to assign valid telescope numbers to additional MSTs in legacy prod6 simulations.#2171
GernotMaier merged 4 commits into
mainfrom
legacy-prod6-mst2

Conversation

@GernotMaier
Copy link
Copy Markdown
Contributor

This is very very fine tuned to the current legacy-style prod6 production with less metadata then the simtools produced files.

Additional MSTs labels 'MST2' are added with IDs beyond the nominal MSTs.

Please test and give feedback.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the legacy sim_telarray metadata fallback logic to handle additional MSTs labeled as MST2 in legacy prod6 files by parsing them from the CORSIKA input card and attempting to map them onto valid MSTS/MSTN telescope names.

Changes:

  • Updated input-card parsing to use a more structured regex and include MST2 entries in the returned telescope list.
  • Added _legacy_merge_msts to rewrite/merge MST2 entries into site-specific MST{S|N}-NN names for legacy guessing.
  • Added unit tests covering MST2 parsing and merge behavior, plus a changelog fragment.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/simtools/simtel/simtel_io_metadata.py Updates legacy telescope-name guessing by parsing MST2 and merging it into valid MSTS/MSTN naming.
tests/unit_tests/simtel/test_simtel_io_metadata.py Extends coverage for parsing MST2 from input cards and adds tests for the merge helper.
docs/changes/2171.feature.md Adds a changelog entry describing the new legacy-prod6 MST2 handling.
Comments suppressed due to low confidence (1)

tests/unit_tests/simtel/test_simtel_io_metadata.py:245

  • This test case appears internally inconsistent with the intended behavior: the input list does not contain a standalone "MSTN-01" entry (it is embedded in a malformed string), but the expected output includes "MSTN-01". As written, _legacy_merge_msts cannot recover "MSTN-01" from that malformed token. Either adjust the test input to include a separate MSTN entry or change the parsing logic to split/clean malformed tokens before processing.
def test_legacy_merge_msts_with_malformed_mst2_entry(monkeypatch):
    msts = ["LSTM-01", 'MSTN-01, "MST2-02']

    monkeypatch.setattr(
        "simtools.utils.names.get_site_from_array_element_name",
        lambda _: "North",
    )

    assert simtel_io_metadata._legacy_merge_msts(msts) == ["LSTM-01", "MSTN-01"]

Comment thread src/simtools/simtel/simtel_io_metadata.py Outdated
Comment thread src/simtools/simtel/simtel_io_metadata.py Outdated
Comment thread src/simtools/simtel/simtel_io_metadata.py Outdated
GernotMaier and others added 2 commits May 7, 2026 09:02
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@ctao-sonarqube
Copy link
Copy Markdown

ctao-sonarqube Bot commented May 7, 2026

@GernotMaier GernotMaier marked this pull request as ready for review May 7, 2026 08:03
@GernotMaier GernotMaier requested a review from orelgueta May 7, 2026 08:03
@orelgueta
Copy link
Copy Markdown
Contributor

I think SCTs are not included. The regex doesn't seem to include that case and running with the following file leads to unknown telescopes:

/pnfs/ifh.de/acs/grid/cta/ctao/MC/Prod6/Paranal/gamma/sim_telarray/7768/Data/002xxx/gamma_20deg_0deg_run002856___cta-prod6-2147m-Paranal-dark+all-scts.simtel.zst

I assume as well that it is by design that telescope_list_common_id is set to zero for telescopes not listed in the identifier repo?

@orelgueta
Copy link
Copy Markdown
Contributor

I think SCTs are not included. The regex doesn't seem to include that case and running with the following file leads to unknown telescopes:

/pnfs/ifh.de/acs/grid/cta/ctao/MC/Prod6/Paranal/gamma/sim_telarray/7768/Data/002xxx/gamma_20deg_0deg_run002856___cta-prod6-2147m-Paranal-dark+all-scts.simtel.zst

Wait, ignore that, my test was run in a python notebook and I didn't notice the cell was not updating. Another reminder why I don't trust those notebooks...
SCTs look fine and I also found MSTS-134 and above which I assume come from those eight MST-NectarCams (no MSTS above 141).

I think it all looks good now. Just the following question remains and I can approve.

I assume as well that it is by design that telescope_list_common_id is set to zero for telescopes not listed in the identifier repo?

@GernotMaier
Copy link
Copy Markdown
Contributor Author

Yes - the common ids are from the identifier repo and set to 0 if a telescope is not found. And we should make more use of those IDs, as string comparison are not already idea.

@orelgueta
Copy link
Copy Markdown
Contributor

OK, perfect. Once this is merged and a new prod container is produced with it, I will run a larger statistics sample for a final test.

@GernotMaier GernotMaier merged commit b78ba81 into main May 7, 2026
18 checks passed
@GernotMaier GernotMaier deleted the legacy-prod6-mst2 branch May 7, 2026 12:10
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.

3 participants