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

Refactor simulation tools configuration #989

Merged
merged 120 commits into from
Jul 8, 2024

Conversation

GernotMaier
Copy link
Contributor

@GernotMaier GernotMaier commented Jun 14, 2024

Simplifies the configuration of simulation tools.

This is a major refactoring PR touching almost all modules relevant for simulation configuration and production. This addresses especially simulate_prod.

Main changes are:

  • removal of configuration by yaml file (as those listed in data/parameters); replaced by configuration via command line (or using configuration files for all command line parameters). There are two parameter files left (camera-efficiency_parameters.yml, corsika_parameters.yml), which will be removed in upcoming PRs
  • re-organiation of all 'runners': there is now a dedicated runner directory to collect all of them
  • introduce module runner_services to collect common functionality
  • removed a lot of obsolete code, unused code; simplified many models (e.g., instead replaced get_output_file, get_log_file, get_histogram_file, etc by get_file(file_type="output").
  • removed application simulate_showers_for_trigger_rates - this is a duplication of simulate_prod.
  • removed a couple of debug statements which were part of loops (to many output statements makes it hard to read)
  • trying to replace module calls like _check_run_result(self, **kwargs) by def _check_run_result(self, run_number=None): - this makes it much clearer
  • rigorous testing of all new functions / modules

Closes #982
Closes #792 (configuration is now completely different and this does not apply anymore)
Closes #911 (configuration files are now handled through the command line and argparser; no metaschema required)

This comment has been minimized.

@GernotMaier GernotMaier marked this pull request as ready for review July 2, 2024 13:54

This comment has been minimized.

This comment has been minimized.

@GernotMaier GernotMaier self-assigned this Jul 5, 2024
help="Run number (actual run number will be 'start_run' + 'run')",
type=int,
required=True,
)
config.parser.add_argument(
Copy link
Collaborator

Choose a reason for hiding this comment

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

All the removed arguments still appear in the docstring above. Why did you remove the config.parser.add_arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, they are moved to the command_line module, as we use this type of parameters for several applications. So the docstring is still fine (as e.g., you want to configure simulate_prod with azimuth or number of runs).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the docstring the parameters are under command_line_parameters. Maybe you can make clear that the values come from the config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is correct, they are command line parameters:

  • any argument defined through Configurator can be set on the command line, by environmental variable or using the --config option and given them all in a configuration file.

This allows us to have let the user decide to configure tools with super long command lines or using a configuration tool.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would say they are converted into command line parameters if they are passed via a configuration file? Maybe you can add this sentence at the beginning somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been changed, as discussed in the chat.

Copy link
Collaborator

@tobiaskleiner tobiaskleiner left a comment

Choose a reason for hiding this comment

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

Thanks @GernotMaier , see few comments.

This comment has been minimized.

@GernotMaier
Copy link
Contributor Author

@tobiaskleiner - thanks a lot for the review. I am waiting for one clarification from your side (on the run numbers / divergent pointing issue). Let me know if there is anything else.

This comment has been minimized.

Copy link

Passed

Analysis Details

0 Issues

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

Coverage and Duplications

  • Coverage 96.10% Coverage (89.00% Estimated after merge)
  • Duplications 0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w

View in SonarQube

@GernotMaier
Copy link
Contributor Author

Thanks again @tobiaskleiner . Let me know if there is anything else.

Copy link
Collaborator

@tobiaskleiner tobiaskleiner left a comment

Choose a reason for hiding this comment

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

All good now. Thanks again @GernotMaier.

@GernotMaier GernotMaier merged commit fb4df6d into main Jul 8, 2024
12 checks passed
@GernotMaier GernotMaier deleted the simulation-tools-configuration branch July 8, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants