New tutorial to calculate radial velocity from multiple measurements - #550

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Thanks for contributing! This looks interesting, but I think you might want to see if the indexing logic can be simplified a bit. At least I don't understand it - but maybe that's just because I don't know that much about the topic.

This looks really promising! Thanks so much for the contribution and we'll work on giving it a robust review ASAP. I'm also going to start a thread in the #learn channel on slack.

Hello @alexbinks!

I realise it has now been a number of years since you submitted this and that you have probably moved on to new projects. However, I think this is a useful notebook and would like to see it added to Learn. To that end I am happy to do the editing to make it conform to our coding standards. I just have two questions about the code itself, and need you to say that you are happy for me to do this (I will link you to the notebook before it goes live so that you get a chance to verify it before we make it public).

My questions are:

1. In the test_mult function you check where the calculated $\Delta RV$ values are greater than the max as: ee = c > d_max, x = np.array(np.where(ee))[0]. Why do you only use the first row of np.array(np.where(ee))?

2. One line later you remove the unphysical values with rv = np.delete(trv, x[0], 0). Why do you only use the first index of x?

I apologise for how long we've let this sit. Let me know what you think of my proposed plan of action.

Thank you for your contribution!

I'll have a look, I'm one of the co-authors after all. I won't get to that until next week though.

As I said in #550 (comment) I thin the indexing loci can maybe a bit simplified, or be explained better, but to answer @ceb8 direct question: ee is an n*n array, so where will return (row, column) information. We just care for the row number x = np.array(np.where(ee))[0], and throw out the column number. A row will typically be listed multiple times, so we just pick the first one (the x[0] in rv = np.delete(trv, x[0], 0).

However, that (a) is hard to understand even for me and (b) it includes a real bug if there is more than one measurement that should be removed @alexbinks: See example below! It probably doesn't happen often in real data, but you should check! I'll change the code of the notebook soon to fix that you you'll have to check your own science applications for this case!

Example for bug:

trv = np.array([10.0, 10.1, 10.2, 10.3, 1000, 1000])
erv = np.array([1.,1.,1.,1., 1., 1.])

c = trv[:, None] - trv[None, :]
ee = c > d_max
 x = np.array(np.where(ee))[0]

which then leads to

np.where(ee)
(array([4, 4, 4, 4, 5, 5, 5, 5]), array([0, 1, 2, 3, 0, 1, 2, 3]))

The code should really remove datapoint 4 and datapoint 5, but x[0] will select the first entry (row 4) only and row 5 slips though.

The easiest would be to use np.unique(x) which will return all unique entries for x, but there might a better way for that test that's also easier to read in the tutorial; I'll make a suggestion shortly.

I made a PR to @alexbinks fork in alexbinks#1
I don't have write permission to his GH, but @ceb8 as a maintainer of astropy-learn you can either take my branch and push it to @alexbinks or just merge my branch or choose to wait for @alexbinks to merge my PR into his branch. Options, options, options, ... but really pick what's easiest for you and what gets us to merge this new content into learn without waiting another few years ;-)