search

UKZN-Astronomy / corrcal

Python/C code for calibration of quasi-redundant arrays. Different (fixed) algorithm from original corrcal
BSD 2-Clause "Simplified" License
1 stars 0 forks source link

Add docstring #5

Closed piyanatk closed 4 years ago

piyanatk commented 4 years ago

The Python code need docstring. @ronniyjoseph, since you volunteered, I assigned you for this task! :) Please work on the restructure branch and submit a pull request when you are ready. Also, please use Numpy docstring format. This is what RAGS (Radio Astronomy Software Group) and many astronomy Python package use, so we should follow along. https://github.com/UKZN-Astronomy/corrcal/projects/4#card-27538686

ronniyjoseph commented 4 years ago

I merged my local documented branch with @piyanatk's restructured branch (AND IT DIDN'T EXPLODE). I've was able to document everything because I haven't played around with everything yet.

piyanatk commented 4 years ago

Great! @Mdlalose and @Zahra-Kader , could you please check the docstring that @ronniyjoseph just added and, let us know if they make sense or if you see anything that could be added. To see this, you have to go to the most recent commit on the restructure branch here https://github.com/UKZN-Astronomy/corrcal/blob/restructure/corrcal/corrcal.py.

Zahra-Kader commented 4 years ago

I took a look at it and it looks great. I will have a more detailed look.

With regards to the lack of use of the get_chisq_dense and get_gradient_dense, I haven't used them but started looking into them yesterday. I think they're going to be useful to me for my debugging since these appear to calculate the chi_sq without redundant blocks or efficient matrix inversion techniques. Out of interest to everyone, I'm trying to figure why low visibility noise in my simulated data gives such a large chi_sq, especially since the high visibility noise gives a low and reasonable chi_sq, and I'm going to see if using get_chisq_dense helps give more reasonable results..If I get anything useful out of this, I will add a docstring. Either way I think these two _dense functions are useful to keep somewhere in the corrcal code for completeness.

ronniyjoseph commented 4 years ago

@Zahra-Kader thanks for looking into that, I think you're right. The arXiv pre-print discusses how c-code actually slows corrcal down for small problems:

For small (∼ 100 antenna) problems with only a few source/redundant vectors, using BLAS/LAPACK actually slows down the code, since the function overhead for e.g. Cholesky decomposition of 2x2 blocks is non-negligible. So, the reference implementation fully self- contained.

And the non-optimised version might be useful as part of some future test.

piyanatk commented 4 years ago

Close as we have sufficient docstring for now