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

Replace model parameter default schema versions by value read from model_parameter.metaschema.yml #1332

Merged
merged 18 commits into from
Feb 3, 2025

Conversation

GernotMaier
Copy link
Contributor

@GernotMaier GernotMaier commented Jan 29, 2025

Addresses the repetition of getting / reading the model parameters schema files and removes the necessity of detailed knowledge about paths and naming of schema files (especially for those in src/simtools/schemas/model_parameters/):

  • introduces a now module simtools.data_model.schema (recommend to start reviewing that)
  • add several of the main schema information (paths and files) to simtools.constants
  • removes / changes wherever possible the code related to schema files to use the generalized approach
  • add a function to get the most recent schema version for the model parameters (or check that a given version actually exists): schema.model_parameter_schema_version. Closes Replace model parameter default schema versions by value read from model_parameter.metaschema.yml #1327
  • add a small improvement to simtools.utils.general.collect_data_from_file to get the Nth document of a multi-document yaml file

@GernotMaier GernotMaier self-assigned this Jan 29, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 22 changed files in this pull request and generated no comments.

Files not reviewed (15)
  • tests/conftest.py: Evaluated as low risk
  • tests/unit_tests/data_model/test_metadata_model.py: Evaluated as low risk
  • src/simtools/applications/validate_file_using_schema.py: Evaluated as low risk
  • src/simtools/applications/convert_all_model_parameters_from_simtel.py: Evaluated as low risk
  • tests/unit_tests/data_model/test_data_reader.py: Evaluated as low risk
  • tests/unit_tests/data_model/test_metadata_collector.py: Evaluated as low risk
  • tests/unit_tests/data_model/test_validate_data.py: Evaluated as low risk
  • src/simtools/model/array_model.py: Evaluated as low risk
  • src/simtools/data_model/model_data_writer.py: Evaluated as low risk
  • src/simtools/data_model/metadata_collector.py: Evaluated as low risk
  • src/simtools/data_model/validate_data.py: Evaluated as low risk
  • src/simtools/layout/array_layout.py: Evaluated as low risk
  • src/simtools/data_model/metadata_model.py: Evaluated as low risk
  • tests/unit_tests/data_model/test_model_data_writer.py: Evaluated as low risk
  • src/simtools/utils/names.py: Evaluated as low risk

This comment has been minimized.

@GernotMaier GernotMaier marked this pull request as ready for review January 30, 2025 08:53
@GernotMaier
Copy link
Contributor Author

Just did a merge from the branch db-simulation-model-refactoring - this should fix the failing integration tests.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Base automatically changed from db-simulation-model-refactoring to main January 31, 2025 10:12

This comment has been minimized.

This comment has been minimized.

from simtools.constants import MODEL_PARAMETER_METASCHEMA, MODEL_PARAMETER_SCHEMA_PATH


def model_parameter_schema_files(schema_directory=MODEL_PARAMETER_SCHEMA_PATH):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest to rename the functions to make their purpose clearer. Maybe get_parameters_and_schema_files, also the following get_schema_path_for_model_parameterand get_validated_schema_version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK - for me it appears clear the schema.model_parameter_schema_files gives me the schema files. But your comment shows that this is not the case for everyone, so I changed it and added everywhere the get_...

@orelgueta
Copy link
Contributor

Note: merges into db-simulation-model-refactoring branch - the 3 integration tests fails until PR #1319 is reviewed an merged

Is this still correct?

@GernotMaier
Copy link
Contributor Author

Note: merges into db-simulation-model-refactoring branch - the 3 integration tests fails until PR #1319 is reviewed an merged

Is this still correct?

No. Removed it.

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, looks good overall. Added a minor comment.

@GernotMaier
Copy link
Contributor Author

@tobiaskleiner - thanks for the review. I've addressed the comment and changed the function names in simtools.data_model.schema. let me know if there is anything else.

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 (93.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

@GernotMaier GernotMaier merged commit 6ea2d5f into main Feb 3, 2025
12 checks passed
@GernotMaier GernotMaier deleted the write-prod5-model-parameters branch February 3, 2025 10:25
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.

Replace model parameter default schema versions by value read from model_parameter.metaschema.yml
3 participants