-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modified Reference Antenna Selection and unit tests #65
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If any of the comments need more explanation feel free to chat to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review for changes made on the initial commit to calprocs.py
and test_calprocs.py
…s with def setup_method, changing to def setup (as in master)
…s with def setup_method, changing to def setup (as in master)
…ts and cleaned up code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice changes! A heads-up: Tasmiyah and I are moving all calprocs
functionality to its own package: https://github.com/ska-sa/katsdpcalproc. You caught me just as I wanted to delete calprocs, but now I'll wait for your PR 😁 Once this is merged, we should move those changes across to the new package as well before removing calprocs
.
ant_indices = np.arange(len(med_pnr_ants)) | ||
median = np.nanmedian(med_pnr_ants) | ||
|
||
mad = scipy.stats.median_abs_deviation(med_pnr_ants, nan_policy='omit') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice that you found this! 😊
If you stumble upon a useful function in an external library, it's good to stop and consider a few things:
- How new is this function?
- Does this introduce a new dependency?
- Will it affect my package's existing dependencies?
Since katsdpcal
already uses scipy
, there's no new dependency to think about. The function is relatively new though, which is why it hasn't been used up to now 😅
Have a look at the scipy docs:
and then at https://pypi.org/project/scipy/#history:
It's from scipy 1.5.0 in 2020, which seems old, but katsdpcal
(mostly from 2017-2019) still accepts scipy >= 0.17
in setup.py
... And scipy 1.5.0 needs Python 3.6, which is newer than the 3.5 specified by katsdpcal.
You have a few options here:
- Quietly bump the Python to 3.6 and scipy to 1.5.0 in setup.py, but ideally we want to run
pyupgrade --py36-plus
to get more benefits. - Go all out and bump it to Python 3.8 (my latest test PR SPR1-3111: Replace nose and asynctest with pytest #67 requires it already).
- Don't use this function and consider the existing functionality in
katsdpsigproc
(NoiseEstMADHost) or just write your own. It's already implemented inkatsdpcal.plotting.amp_range
, for example.
It's also a good idea to multiply the MAD by 1.4826 to convert it into a "normal" standard deviation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think its best to implement my own function. I am going to calculate the MAD as follows
pnr_values = med_pnr_ants[~np.isnan(med_pnr_ants)]
med = np.median(np.abs(pnr_values))
mad = np.median(np.abs(pnr_values - med))
mad_threshold = (med - mad)
I still need to think about the normalised MAD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always leave a comment in the code that the scipy function could be used once we upgrade to Python 3.6+ in case we forget. Or implement a function called "median_abs_deviation" that can serve both your code as well as katsdpcal.plotting.amp_range
, while also having the signature of the scipy function so that one day you can just delete your function and replace with the similar-named scipy function.
@@ -789,7 +788,7 @@ def test_no_ac(self): | |||
|
|||
|
|||
class TestInterpolateSoln(unittest.TestCase): | |||
"""Tests for :func:`katsdpcal.calprocs.interpolate_soln`""" | |||
"""Tests for :func:`katsdpcal.calprocs.interpolate_soln""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You accidentally deleted a backtick here (needed for Sphinx doc reference).
|
||
|
||
class TestSelectMedDeviation(unittest.TestCase): | ||
"""Tests for :func:`katsdpcal.calprocs.select_med_deviation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You accidentally deleted a backtick here (needed for Sphinx doc reference).
|
||
|
||
class TestBestRefAnt(unittest.TestCase): | ||
"""Tests for :func:`katsdpcal.calprocs.best_refant""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You accidentally deleted a backtick here (needed for Sphinx doc reference).
@@ -70,6 +70,7 @@ class PingTask(control.Task): | |||
def __init__(self, task_class, master_queue, slave_queue): | |||
super().__init__(task_class, master_queue, 'PingTask') | |||
self.slave_queue = slave_queue | |||
self.master_queue = master_queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary since the super()
above already does it (master_queue
lives in the control.Task
base class).
@@ -95,8 +96,9 @@ class BaseTestTask: | |||
""" | |||
|
|||
def setup(self): | |||
self.master_queue = self.module.Queue() | |||
self.slave_queue = self.module.Queue() | |||
module = multiprocessing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why you did this... Did you try to run this with pytest
by any chance? The tests work as expected for me without this. I know it looks strange, being a Merry construct and all, because I also had to figure this out just this week 😂
The module
attribute is set by the actual (derived) test classes below: TestTaskMultiprocessing
and TestTaskDummy
. As the name implies, this class is just the base for these tests containing common functionality. The module
attribute is not common and will be set differently for each set of tests. By hardwiring multiprocessing
here, you are dropping the TestTaskDummy
checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran locally with pytest
that needed this change, and had neglected to change back when I reran test_control.py
with nosetests
thanks for the spot.
Co-authored-by: Ludwig Schwardt <ludwig@ska.ac.za>
I suggest that we squash the branch to a single commit when we eventually merge this. That will avoid adding commits that just add lines and then delete it again. |
This PR contains the modified selection of reference antenna as per the initial investigation done in SPR1-1964
A
select_med_deviation_pnr_ants
function has been added to calculate the median deviation of antenna peak-to-noise (PNR) values, this function returns a boolean array of antenna values that have a relatively good PNR, i.e one deviation above the median deviation. Thebest_refant
, which is the function that calculates the PNR across antennas, has been modified to call theselect_med_deviation_pnr_ants
and thereafter return the corresponding longest baseline solutions for the reference antenna selection.The
test_calprocs.py
contains the corresponding unit tests for the new solution.