Separate TelescopeModel and SiteModel for sim_telarray configuration writer #1489
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
orelgueta
reviewed
Apr 9, 2025
| ) | ||
|
|
||
| @property | ||
| def config_file_directory(self): | ||
| """Directory for configure files. Configure, if necessary.""" | ||
| if self._config_file_directory is None: | ||
| self._set_config_file_directory_and_name() | ||
| return self._config_file_directory | ||
| return self._config_file_directory or self._set_config_file_directory_and_name() |
There was a problem hiding this comment.
Wouldn't this make the code above it checking if None redundant? I suggest applying the same lazy init to the config_file_path while we are at it.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1 similar comment
Analysis Details0 Issues
Coverage and Duplications
Project ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w |
|
@orelgueta - thanks for the review! Please have another look, I think I've followed all your suggestions. |
orelgueta
reviewed
Apr 9, 2025
| if self._config_file_directory is None: | ||
| self._set_config_file_directory_and_name() | ||
| return self._config_file_directory or self._set_config_file_directory_and_name() | ||
| return self._config_file_directory |
There was a problem hiding this comment.
I thought the opposite, i.e., that
return self._config_file_directory or self._set_config_file_directory_and_name()
would cover both cases and you won't need
if self._config_file_directory is None:
self._set_config_file_directory_and_name()
But it doesn't matter, we can keep it like this.
orelgueta
approved these changes
Apr 9, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TelescopeModel and SiteModel have not been separated in the ModelParameter and SimtelConfigWriter routines - site parameters have been added simply to the telescope model. This had some side effects, like that checks for site parameters where necessary when writing telescope parameters.
This PR refactors the code and introduces a clear separation of TelescopeModel and SiteModel. Unfortunately, this required quite some changes, but I think the code is now easier to read and definitely more consistent.
Notes:
ModelParameter.get_config_filehas been removed and replaced byget_config_file(a property) andwrite_sim_telarray_config_file(this hopefully clarifies from where we call the config file writer).array_triggersparameter as part of TelescopeModel in the code. These will be fixed in a follow up PR.