Closed steven-murray closed 1 year ago
Base: 99.13% // Head: 99.15% // Increases project coverage by +0.01%
:tada:
Coverage data is based on head (
c2c9c9c
) compared to base (8ae4dee
). Patch coverage: 100.00% of modified lines in pull request are covered.
:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
@bhazelton I think this is ready now. I'm getting that three tests fail, and I'm not sure why. It doesn't seem to have anything to do with the code that I modified (but I'm likely wrong, of course).
I've fixed the test issues in #409, so once that is merged we can rebase this. Note that that PR includes some new pre-commit checks, so after rebasing, you'll probably want to run pre-commit to make sure checks will pass (checks do not automatically run when you rebase like the they do when you commit). I'm happy to help with or do the rebase if you'd like.
@bhazelton I've done the rebase and also covered some lines that weren't covered. The things that are failing are weird new warnings from pytest, that I don't understand (nothing to do with my additions). The project coverage went down but the patch coverage is 100% so I can't do anything about that.
@bhazelton I think this is ready for review again -- any idea on the min deps error?
@bhazelton I think this is good to go again -- the warnings test is only failing on the codecov upload.
Description
This PR adds the following performance-enhancing features in the context of setting up a simulation from config:
select:
in the telescope YAML file, to which can be addedfreq_range
,az_range
andza_range
. For instance, if a simulator only requires the beam above the horizon, they may want to setza_range: [0, 95]
(where we use 95 so that interpolation to the horizon doesn't quite hit the edge). It also adds the possibility of aselect:
parameter in the main obsparam file, under thetelescope:
heading. Here, I've only added one possible parameter to theselect:
heading, which isfreq_buffer
. If set, this will pass through its information so that only frequencies from(freq_array.min - buffer, freq_array.max + buffer)
are read in (wherefreq_array
is the UVData frequencies). It seems reasonable that only frequencies surrounding those that will actually be simulated should be read in (again, with some buffer to enable interpolation with k>1).run_check_acceptability
which can double peak RAM)blt_order
attribute of the UVData upon setup, which can help simulators know if the blt axis needs to be reordered to suit their purposes.INFO
-level log-statements throughout the setup. These shouldn't general be shown, but can be switched on by the user in application code. I am open to suggestions on which of these statements are really necessary and whether you'd prefer a different log-level (or different logging mechanism entirely).NOTE: this PR also pins pyradiosky<0.2, due to API changes in pyradiosky that break unrelated code in pyuvsim (unrelated to this PR, that is). That will be fixed by #417.
Motivation and Context
This PR was developed alongside trying to get the H4C simulations run on GPU (using
hera_sim
callingvis_gpu
). The main concerns here were peak RAM usage and timing performance.The RAM usage is a particular concern because there is quite a limited amount of RAM available on the host of the GPU nodes (23.75GB). The simulations we're running are reasonably large (1 freq, 17280 times, 8000 baselines, 4 pols), but should easily fit in this amount of RAM. However, there were several problems with the setup that meant that the peak RAM would exceed this amount. This PR significantly reduces this peak RAM (although, in the end I still had to run the simulations in chunks of 5760 times each).
The wall-time performance is another concern that was previously overlooked. Simulations of this size on the CPU take ~10-20 hours, so the setup/write time is negligible. However, on the GPU, the simulation takes ~15min, which makes every minute of setup time undesirable. Using the changes in this PR (and some changes in
hera_sim
to accompany it) I was able to reduce setup/check/write time from ~30min to mere seconds.Types of changes
Checklist:
For all pull requests:
New feature checklist: