Add decliniation line support in production grid#2075
Add decliniation line support in production grid#2075tobiaskleiner wants to merge 26 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds RA/Dec declination-line support to production grid generation and introduces a new plotting utility to visualize production-grid points in both local Alt/Az and equatorial RA/Dec projections.
Changes:
- Extend
GridGenerationto supportcoordinate_system="ra_dec"with (a) native declination-line/hour-angle sampling and (b) explicit RA/Dec axes mode. - Add
ProductionGridPlotterplus new CLI entrypointsimtools-plot-production-grid, including docs and integration configs. - Add/extend unit tests and test resources for RA/Dec grid generation and production-grid plotting.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/simtools/production_configuration/generate_production_grid.py |
Adds RA/Dec grid generation modes and lookup-table interpolation path for RA/Dec workflows. |
src/simtools/production_configuration/plot_production_grid.py |
New plotting module for production-grid sky projections with optional RA/Dec guide tracks. |
src/simtools/applications/plot_production_grid.py |
New CLI application wiring site metadata + plotter invocation. |
src/simtools/applications/production_generate_grid.py |
Updates CLI docs/help to describe new RA/Dec behavior. |
tests/unit_tests/production_configuration/test_generate_production_grid.py |
Adds RA/Dec-mode unit tests and refactors GridGeneration setup in tests. |
tests/unit_tests/production_configuration/test_plot_production_grid.py |
New unit tests covering normalization, inferred tracks, and output generation. |
tests/resources/production_grid_generation_axes_definition_radec.yml |
New RA/Dec axes definition fixture for tests/integration. |
tests/integration_tests/config/production_generate_grid_radec.yml |
New integration workflow for RA/Dec grid generation. |
tests/integration_tests/config/plot_production_grid.yml |
New integration workflow for plotting production grids. |
docs/source/user-guide/applications/simtools-plot-production-grid.rst |
New user-guide stub for the plotting CLI. |
docs/source/user-guide/applications.md |
Adds the new plotting CLI to the applications list. |
docs/source/api-reference/production_configuration.md |
Adds API docs entry for production_configuration.plot_production_grid. |
pyproject.toml |
Registers simtools-plot-production-grid console script. |
src/simtools/production_configuration/generate_production_grid.py
Outdated
Show resolved
Hide resolved
|
Example plot: |
tests/unit_tests/production_configuration/test_generate_production_grid.py
Show resolved
Hide resolved
|
@tobiaskleiner - can you address the copilot comments first before my review? |
|
GernotMaier
left a comment
There was a problem hiding this comment.
Thanks a lot for these additions - very nice!
Below mostly minor comments - I think the question on what time format we use and if we should use a table as output is most important.
|
|
||
| .. code-block:: console | ||
|
|
||
| simtools-production-generate-grid --site North --model_version 6.0.0 \ |
There was a problem hiding this comment.
What is telescope_id 1? We usually use e.g. MSTN-05, etc.
| --axes tests/resources/production_grid_generation_axes_definition.yml \ | ||
| --coordinate_system zenith_azimuth \ | ||
| --lookup_table tests/resources/corsika_simulation_limits_lookup.ecsv \ | ||
| --telescope_ids 1 |
There was a problem hiding this comment.
Could we change in the generated output file:
- radius to scatter radius
Why does energy threshold have one additional level:
"energy_threshold": {
"lower": {
"value": 0.0045877057774406345,
"unit": "TeV"
}
(e.g. radius is an upper limit, but does not have an 'upper' level.
Also suggest (again) that we apply reasonable rounding for all output.
Why did we actually choose a json file and not a table output? One row per grid point? It is hard to read this way with a lot of duplication.
Maybe also consider (in this or a follow up PR) how to add some minimal information about how the file has been generated.
| The coordinate system for the grid generation ('zenith_azimuth' or 'ra_dec'). | ||
| In ``ra_dec`` mode, observing location/time are used to build sky directions and | ||
| derive corresponding zenith/azimuth values for interpolation. | ||
| observing_time (str, optional, default=now) |
There was a problem hiding this comment.
Note sure about the choice of local sideral time in term of radec. I think in general the time format for observations is UTC (and obviously need to be converted to sideral time in the code).
Is observation_time needed for the zenith_azimuth coordinate system? If not, make it clear in the help.
| grid_points_file: tests/resources/production_grid_points.json | ||
| output_path: simtools-output | ||
| site: North | ||
| model_version: 6.0.0 |
There was a problem hiding this comment.
Sometimes 6.0.0 and sometimes 6.0.2. Should we try and use 6.0.2 consistently?
| output_file: grid_output_radec.json | ||
| output_path: simtools-output | ||
| site: North | ||
| telescope_ids: [] |
There was a problem hiding this comment.
It is a bit confusing why a telescope_id list is needed (even an empty list). Is it really needed or just there because this tool also derive energy thresholds, etc?
|
|
||
| direction_points = [] | ||
| for declination in np.arange(-90.0, 91.0, 1.0): | ||
| cos_dec = abs(np.cos(np.deg2rad(declination))) |
There was a problem hiding this comment.
Do you need the abs? The cos is >0 for -90,90.
| All explicit RA/Dec combinations defined by the input axes are preserved, | ||
| even when their transformed Alt/Az coordinates fall below the local horizon. | ||
| """ | ||
| if self.observing_time is None: |
There was a problem hiding this comment.
Repetition of line 244 - do we need to check this several times?
| } | ||
|
|
||
| def _generate_grid_radec_mode(self): | ||
| """Generate grid points for RA/Dec mode using uniform declination lines.""" |
There was a problem hiding this comment.
What does 'uniform' mean in the context? Do you mean that we apply equal binning in hour angle along the declination lines?
| site_location_height : float or astropy.units.Quantity | ||
| Site height in meters. | ||
| observation_time : str | ||
| Observation time in ISO format. |
There was a problem hiding this comment.
You might want to the the time type - sideral?
| return [entry["grid_point"] for entry in data] | ||
| return list(data) | ||
|
|
||
| msg = f"Unexpected JSON structure in {self.grid_points_file}" |
There was a problem hiding this comment.
We should consider adding a json schema if we keep json format and test the schema (as in many other places) - but as mentioned above, maybe we should consider using tables?
|
@tobiaskleiner - can you also have a quick look at the coverage - 24 uncovered lines appears to be a lot (maybe not)? https://sonar-ctao.zeuthen.desy.de/component_measures?id=gammasim_simtools_0d23837b-8b2d-4e54-9a98-2f1bde681f14&metric=new_coverage&pullRequest=2075&view=list&selected=gammasim_simtools_0d23837b-8b2d-4e54-9a98-2f1bde681f14%3Asrc%2Fsimtools%2Fproduction_configuration%2Fplot_production_grid.py |
Adds RA/Dec declination-line support to production grid generation and introduces a new plotting utility to visualize production-grid points in both local Alt/Az and equatorial RA/Dec projections.