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

Fix testeff again #1177

Merged
merged 6 commits into from
Oct 1, 2024
Merged

Fix testeff again #1177

merged 6 commits into from
Oct 1, 2024

Conversation

orelgueta
Copy link
Contributor

The summary below was written by copilot. The gist is that

  • the necessary file is now exported to the model directory, though the function in model_parameters.py is a bit ugly.
  • I switched around the option from apply the correction to skip the correction which makes more sense.
  • An expected failing test is skipped and will be fixed as part of a separate issue that is already open (Customize testeff command for SST #1155)

This pull request includes several changes to the simtools package to update the handling of the NSB spectrum correction and add new functionality for exporting model files. The key changes involve renaming a parameter, updating method calls, and adding new methods and tests to support these updates.

Parameter Renaming and Method Updates:

New Functionality:

Tests:

…ory if the correction is applied. Change the command line option to be skip instead of apply to follow the logic better. Skip the SST test because the mirrors are not implemented yet. Add tests for new function.
@GernotMaier
Copy link
Contributor

The unit tests fail at a surprising place, do you want to fix that first?

@orelgueta
Copy link
Contributor Author

I thought it was a hiccup last night, so I reran it and it still happens. Completely weird considering I see a correct value in the DB and locally this test passes. I will see what I can do.

…date we get 79 photons (it is expected that protons would lead to less photons, so this should be fine)
@orelgueta
Copy link
Contributor Author

I fixed the integration test. The change in the number of photons on the ground is not entirely expected, I didn't think the change in sim_telarray would lead to that, but it's not an issue I think so I relaxed the requirement instead.

The unit test I still can't reproduce, so I will wait to see if it happens again on github.

Copy link

Passed

Analysis Details

0 Issues

  • Bug 0 Bugs
  • Vulnerability 0 Vulnerabilities
  • Code Smell 0 Code Smells

Coverage and Duplications

  • Coverage 100.00% Coverage (91.80% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w

View in SonarQube

@orelgueta
Copy link
Contributor Author

Unit tests are fixed, not sure why it was never an issue before, probably due to a random order of tests running. Good that we found this issue anyway.

Integration tests are also expected to pass, but you can wait for them before reviewing of course.

Copy link
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!

if "simtools-validate-camera-efficiency_SSTS" == full_test_name:
pytest.skip(
"The test simtools-validate-camera-efficiency_SSTS is skipped "
"since the fake SST mirrors are not yet implemented (#1155)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like the expression 'fake'. What does it mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to provide a list of mirrors in the "standard style" of sim_telarray for SST. KB called it fake_astri_mirrorpos.dat, so I did the same here. Either way, this comment will disappear when #1155 is fixed.

@orelgueta orelgueta merged commit 4ef1cbd into main Oct 1, 2024
13 checks passed
@orelgueta orelgueta deleted the fix_testeff_again branch October 1, 2024 09:35
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