Read site names from list of array elements #928
Conversation
There was a problem hiding this comment.
Hi @GernotMaier, I provide a couple of comments but in general all good here.
simtools/utils/names.py
Outdated
| The list of sites is derived from location of available array elements | ||
| Return a dictionary for compatibility with the validation routines. |
There was a problem hiding this comment.
I found this explanation a bit confusing. Could you please improve it a bit?
E.g. "The list of sites is derived from the array element schema file."
"Returns a dictionary that is compatible with..."
There was a problem hiding this comment.
Good point, improved it.
| @@ -34,7 +34,7 @@ | |||
| # - {CLASS: null, DESCRIPTION: null, ID: null, SITE: null, SUBTYPE: null, TYPE: null} | |||
| # DOCUMENT: | |||
| # - {CREATION_TIME: null, ID: null, TITLE: null, TYPE: null, URL: null} | |||
| # INSTRUMENT: {CLASS: null, DESCRIPTION: null, ID: null, SITE: CTA-North, SUBTYPE: null, TYPE: null} | |||
There was a problem hiding this comment.
This made me realize that we still have a lot of references to "CTA" in the code, whereas it should now be "CTAO". Should we open an issue to change that?
There was a problem hiding this comment.
I wouldn't spend time on it now but only when a new version of the metadata documents is released. There might be other changes to be done.
There was a problem hiding this comment.
Ok, I just created an issue such that we don;t forget about it. No need to work on that now
| "South": ["paranal", "south", "cta-south", "ctao-south", "s"], | ||
| "North": ["lapalma", "north", "cta-north", "ctao-north", "n"], | ||
| "South": ["south"], | ||
| "North": ["north"], |
There was a problem hiding this comment.
Do we really need such a validation now that only one string name is accepted?
There was a problem hiding this comment.
The validation still works with a dict, as the general validation routine in names.py (names._validate_name) requires a dict.
|
@VictorBarbosaMartins - thanks for looking into it. I've improved the docstring, let me know if I should do anything else. |
This is a follow-up PR to reduce the number of hard-wired values in
names.py- this time removing the list of sites and replaces it by reading the sites from the array elements file insimtools/schemas/array_elements.yml.Remove also the ambiguity in site definitions (it is South/North (or whatever is defined in simtools/schemas/array_elements.yml) - not CTAO-South, not CTA-South, Paranal, paranal etc).