Major update of Telescope class #631
Conversation
…stations (antennas) 🕜
| f"""CSV has {dataframe.shape[1] - cls.SOURCES_COLS + 1} | ||
| too many rows. The extra rows will be cut off.""" | ||
| f"CSV has {dataframe.shape[1] - cls.SOURCES_COLS + 1} " | ||
| f"rows too many. The extra rows will be cut off." |
There was a problem hiding this comment.
line 829 doesn't need to be an f-string.
| new_latitude = lat + (east_relative / r_earth) * (180 / np.pi) | ||
| new_longitude = long + (north_relative / r_earth) * (180 / np.pi) / np.cos( | ||
| long * np.pi / 180 | ||
| r_earth = 6378100 # from astropy (astropy.constants.R_earth.value) |
| :rtype: karabo.simulation.telescope.Telecope | ||
| :raises: ValueError if instr_name is not a valid RASCIL telescope | ||
| """ | ||
| config: Configuration = create_named_configuration(instr_name) |
There was a problem hiding this comment.
does specifying the dtype Configuation do anything? If not (I assume because the function should return just this type), I think removing would be a better idea. Not sure what the implication of this is in case the function is changing it's return type for each use-case.
|
|
||
| # there are only stations in the rascil files no antennas | ||
| # we add a dummy antenna | ||
| telescope.add_antenna_to_station(i, 0.1, 0.1) |
There was a problem hiding this comment.
I don't understand why both horizontal x & y need to be 0.1. Can you elaborate?
| @@ -485,7 +536,7 @@ def write_to_disk(self, dir_name: DirPathType, *, overwrite: bool = False) -> No | |||
| ): # for OSKAR & `overwrite` security purpose | |||
| err_msg = f"{dir_name=} has to end with a `.tm`, but doesn't." | |||
| raise RuntimeError(err_msg) | |||
| with write_dir(dir=dir_name, overwrite=overwrite) as wd: | |||
| with write_dir(dir=dir_name, overwrite=True) as wd: | |||
There was a problem hiding this comment.
I think this is an accident? If not, what's the motivation behind ignoring the fun-arg?
| # @pytest.mark.parametrize("site_name, num_stations", rascil_telesecopes_to_test) | ||
| # def test_num_of_baselines(site_name, num_stations): | ||
| # site: Telescope = | ||
| # Telescope.constructor(site_name, backend=SimulatorBackend.RASCIL) | ||
| # num_baselines = num_stations * (num_stations-1) // 2 | ||
| # assert len(site.get_baselines_wgs84()) == num_baselines |
There was a problem hiding this comment.
Why out-commented? If not needed, I think just removing it would be better?
| assert len(baseline_wgs) == 134 | ||
|
|
||
|
|
||
| def test_telescope_baseline_length(rascil_telescope): |
There was a problem hiding this comment.
I think adding type-hint here would be an improvement. In case someone does a refactoring on get_stations_wgs84, it would be catched automatically. Otherwise you'll just see the fail in the failing CI-pipeline.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
+ Coverage 70.76% 70.97% +0.21%
==========================================
Files 55 55
Lines 5879 5905 +26
==========================================
+ Hits 4160 4191 +31
+ Misses 1719 1714 -5 ☔ View full report in Codecov by Sentry. |
Baseline functions of the Telescope class (e.g.
max_baselineorget_stations_wgs84) now have the same output for OSKAR and RASCIL backends. Thus, the two code examples in issue #629 now work as expected. The main work was done inTelescope::constructor()function.Other things that have been changed or adapted:
get_baselines_wgs84toget_stations_wgs84because it always returned coordinates of stations, not baselinesnumpy.loadtxt()with more robust class method__read_layout_txt()(inTelescope::_get_station_info)test_telescope_baselines.py