-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improve position coordinate transformation and add json reader / writer #968
Conversation
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.
2. ground system (similar to sim_telarray system with x-axis pointing toward \ | ||
geographic north and y-axis pointing towards the west); altitude relative \ | ||
to the CORSIKA observation level. | ||
2. ground system (similar to sim_telarray system with x-axis pointing toward geographic north |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can add here that the z coordinate (altitude) is the relative coordinate with the additional corsika sphere center added. (not the ground level)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I've added one sentence to make this clearer:
Altitude is the height of the elevation rotation axis (plus some possible mirror offset).
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @GernotMaier. I provide some comments here. In general I think it is already in a good state.
@@ -152,12 +146,22 @@ def main(): | |||
logger = logging.getLogger() | |||
logger.setLevel(gen.get_log_level_from_user(args_dict["log_level"])) | |||
|
|||
metadata = MetadataCollector(args_dict=args_dict, data_model_name=data_model_name) | |||
json_type = args_dict.get("input", "").endswith(".json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not passing any --input
leads to an error that is not yet dealt with in the code, I suggest including a check to avoid this:
simtools-convert-geo-coordinates-of-array-elements --export ground --select_assets LSTN --site South
Traceback (most recent call last):
File "/workdir/env/bin/simtools-convert-geo-coordinates-of-array-elements", line 8, in <module>
sys.exit(main())
^^^^^^
File "/workdir/external/simtools/simtools/applications/convert_geo_coordinates_of_array_elements.py", line 149, in main
json_type = args_dict.get("input", "").endswith(".json")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'endswith'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually the opinion that it is dealt with: you get a an error message telling you that --input
should not be None
.
I am aware that we have to discuss on the command line configuration of the tools and decide if we need to work the "required" flags or not. Most tools are in the meanwhile quite complex, and parameters interfere. Either we put a effort in getting this right (which is very hard) or we put some trust in the user. This is a follow up item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually do not get an error message telling you that --input
should not be None
. On the contrary, we get an error that a method we tried to use does not exist, which is an issue. These messages are not the same. A simple try and except statement with the message "--input
should not be None
" replacing the error message would already correct this.
to the CORSIKA observation level. | ||
2. ground system (similar to sim_telarray system with x-axis pointing toward geographic north | ||
and y-axis pointing towards the west); altitude relative to the CORSIKA observation level. | ||
Altitude is the height of the elevation rotation axis (plus some possible mirror offset). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me what "plus some possible mirror offset" means. I think this refers to a possible shift due to the mirror shape, but it does not inform whether the shift is already included in the model, or how much is the shift? Should the user worry about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a parameter called "mirror_offset", this is what is mean it. I think "mirror offset" is a good description for the offset of the mirror. Do you have a better suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My better suggestion is to remove (plus some possible mirror offset)
. If this is an information the user needs, then I would pass the complete information, otherwise I think this piece of information is not needed here. This confuses more than adds to the docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VictorBarbosaMartins - thanks a lot for the review. I have picked the items related to this PR.
to the CORSIKA observation level. | ||
2. ground system (similar to sim_telarray system with x-axis pointing toward geographic north | ||
and y-axis pointing towards the west); altitude relative to the CORSIKA observation level. | ||
Altitude is the height of the elevation rotation axis (plus some possible mirror offset). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a parameter called "mirror_offset", this is what is mean it. I think "mirror offset" is a good description for the offset of the mirror. Do you have a better suggestion?
@@ -152,12 +146,22 @@ def main(): | |||
logger = logging.getLogger() | |||
logger.setLevel(gen.get_log_level_from_user(args_dict["log_level"])) | |||
|
|||
metadata = MetadataCollector(args_dict=args_dict, data_model_name=data_model_name) | |||
json_type = args_dict.get("input", "").endswith(".json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually the opinion that it is dealt with: you get a an error message telling you that --input
should not be None
.
I am aware that we have to discuss on the command line configuration of the tools and decide if we need to work the "required" flags or not. Most tools are in the meanwhile quite complex, and parameters interfere. Either we put a effort in getting this right (which is very hard) or we put some trust in the user. This is a follow up item.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
Analysis Details2 IssuesCoverage and DuplicationsProject ID: gammasim_simtools_AY_ssha9WiFxsX-2oy_w |
This PR adds a couple of improvements to the position coordinate transformations:
tests/integration_tests/config/convert_geo_coordinates_of_array_elements_ground_to_utm_json.yml
simtel_config_reader
tomodel_data_writer
(simple move; no change in functionalityprint_array_elements
toconvert_geo_coordinates_of_array_elements
Add a small improvement to the integration tests to the function of
compare_json_files
: for simtools-db-type json files where a list of float is encoded as string (similar to sim_telarray; e.g.,5.5 3.2 7.6
). Used np.all_close to compare those floats.Closes #924