Time profiling pull request #146
Conversation
pyuvdata/miriad.py
Outdated
| if run_check: | ||
| self.check(run_check_acceptability=run_check_acceptability) | ||
| # if run_check: | ||
| # self.check(run_check_acceptability=run_check_acceptability) |
There was a problem hiding this comment.
Are you sure you want the check lines to be commented out? That seems like a bad idea.
There was a problem hiding this comment.
Whoops.... Yes, I cut those from mine because it was running slow and didn't want to address it, but I'll put them back and run the profiler again.
I don't think it'll break, since write_miriad also runs the acceptability check.
pyuvdata/uvdata.py
Outdated
| for ind, jd in enumerate(self.time_array): | ||
| # calculate ra/dec of phase center in current epoch | ||
| obs.date, obs.epoch = self.juldate2ephem(jd), self.juldate2ephem(jd) | ||
| precess_pos.compute(obs) | ||
| ra, dec = precess_pos.a_ra, precess_pos.a_dec | ||
|
|
||
| # generate rotation matrices | ||
| m0 = a.coord.top2eq_m(self.lst_array[ind] - obs.sidereal_time(), latitude) | ||
| m0 = a.coord.top2eq_m(self.lst_array[ind] - obs.sidereal_time(), latitude) ##TODO This is a bottleneck because top2eq_m involves a matrix inversion. |
There was a problem hiding this comment.
Do you have a plan to address this? I think marking it as a bottleneck is usefully informative, but the TODO label gets grabbed by some editors and should only be used if there's a plan to fix it.
There was a problem hiding this comment.
What if I just remove the TODO label? I haven't found a way to address it yet, but maybe someone can? I have another line marked as a bottleneck in read_miriad().
There was a problem hiding this comment.
I don't have time to do it now, but one way to at least reduce the bottleneck, would be to only calculate m0 for unique times, rather than for all blts.
There was a problem hiding this comment.
The other thought I have on this is that we should consider using astropy rather than aipy (which uses pyephem under the hood I think).
There was a problem hiding this comment.
Aipy doesn't use pyephem for this particular function. In fact, it's a rather simple function which just just inverts eq2top_m, which simply records the matrix using sin and cos. I'd be tempted to write a small util function in pyuvdata to write down the inverse directly, which is equally easy.
|
Note to self: before merging this branch, we need to check the phasing outputs against master - be sure we haven't changed this because we don't have a valid test yet. |
|
I think that when this PR is merged, we should put out a new pip package, which requires bumping the version number. Any opinions on whether it should be v1.2 or v1.1.1? |
adampbeardsley
force-pushed
the
time_profiling
branch
from
7d33414
to
45a030b
Compare
|
Because this branch is getting close to merging, I went ahead and did a comparison of phasing with master. It's hard to make an actual unit test... so I just used master to read in a couple files, did various phasing and unphasing operations, writing output files throughout. I then used this branch to repeat those, and compare with the save files throughout. What I learned is that there are a couple places where vectorizing the arithmetic caused intermediate quantities to be cast as float32 instead of float64 (when using uvws, which are float32). This caused small errors in uvws, which were small enough that they passed our tolerance, but translated to unacceptable errors in the data. I was able to make a couple changes locally to make sure everything is cast as float64, and everything works. This is making me think that we actually want the uvws always stored as float64. I'm going to start a new issue for this, but I don't think it's stopping for the time being. I'll push my local changes, just to match master, but they should go away if we do eventually use float64. |
Improvements in speed for read/write operations.