Simplify file naming in names.py #835
Conversation
GernotMaier
changed the base branch from
main
to
dev-simulation-model-elements-naming
GernotMaier
changed the base branch from
dev-simulation-model-elements-naming
to
main
simtools/camera_efficiency.py
Outdated
| @@ -193,7 +183,7 @@ def analyze(self, export=True, force=False): | |||
| """ | |||
| self._logger.info("Analyzing CameraEfficiency") | |||
|
|
|||
| if self._file_results.exists() and not force: | |||
| if self._file["results"].exists() and not force: | |||
There was a problem hiding this comment.
Better to use if "results" in self._file for dict. Also codecov complains about missing test. Maybe you can add tests? Also for the following two lines 225 and 303.
There was a problem hiding this comment.
Done (your first point).
I am not adding tests - all what I have changed is this one line which triggered codecov. No new logic has been introduced.
Both camera_efficiency and ray_tracing modules need a good review. I had a quick look, and it is not straight forward to add tests to those rather long functions. I do think this requires far more attention than adding a quick test to make codecov happy.
There was a problem hiding this comment.
Ok I agree. This would then probably go in a seperate PR. If you agree we can open an issue to adress this.
There was a problem hiding this comment.
Yes and no. We have all the tools like pycoverage and codecov to tell us the missing tests. Do we need to open additionally issues on that? I am really not sure.
| this_mirror if self.config.single_mirror_mode else None, | ||
| self.label, | ||
| "photons", | ||
| photons_file_name = names.generate_file_name( |
There was a problem hiding this comment.
Line not covered by test.
There was a problem hiding this comment.
See comment above, same thing.
simtools/utils/names.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| file_type: str | ||
| Type of file (e.g., config, output, plot) | ||
| key: str |
There was a problem hiding this comment.
This should be named suffix. Description is not matching.
There was a problem hiding this comment.
Exactly. Changed it.
| @@ -12,7 +12,7 @@ | |||
|
|
|||
| def test_reading_simtel_file(args_dict, io_handler): | |||
| test_file = io_handler.get_input_data_file( | |||
| file_name="photons-North-LSTN-01-d10.0-za20.0-off0.000_validate_optics.lis.gz", | |||
| file_name="photons-North-LSTN-01-d10.0km-za20.0deg-off0.000deg_validate_optics.lis.gz", | |||
There was a problem hiding this comment.
Why do we use here 0.000deg for 0 offset, but above 000deg for 0 azimuth angle?
There was a problem hiding this comment.
This is motivated by:
- zenith and offset angle needs to be set on a sub-degree accuracy
- azimuth not - it is probably impossible to see a difference between a shower coming from 180 or 185 deg.
I am generally not a friend of putting metadata in the file name. This information should be in a metdata header. We should fix this at some point in the future.
There was a problem hiding this comment.
Yes this makes sense, but do we really need milli degree accuracy for offset? Agree to put this in the metadata in the future.
There was a problem hiding this comment.
We actually have simulated at 0.125 deg offset at some point, so this is not completely wrong.
There was a problem hiding this comment.
Ok thanks, everything is clear then. Going to mark it as approved.
| ) | ||
| == "camera-efficiency-table-North-MST-FlashCam-D-za040deg_azm000deg_test.ecsv" | ||
| == "camera-efficiency-table-North-MSTN-za40.0deg_azm000deg_test.ecsv" |
There was a problem hiding this comment.
Related to previous comment, also seen later a couple of times.
There was a problem hiding this comment.
Same reply - I've commented on zenith vs azimuth accuracy. Let me know if this makes sense, easy to change it to azm000.0deg, but I don't think it makes sense (and python is horrible in getting the accuracy right)
|
@tobiaskleiner - thanks for the comments and suggestions! Added them or commented on the issues. Let me know if this is ok. |
|
@tobiaskleiner - thank you! |
This PR addresses issue #798:
names.pyfor output files from especially camera_efficiency and ray tracing is very repetitive. Improved this a bit, but a condensation into a single function is hardly possible. I've simplified as much as possible.