Update frequency_correction.py #47
Conversation
Many changes made Added module for automatic identification of a target to use for spectral registration Fixed bug in the way the subset of the frequency range was identified Added additional input to spectral registration module to enable limiting the time range used for alignment Quality of spectral registration is significantly improved over previous version
There was a problem hiding this comment.
Hi Laura, really pleased to see you are back working on this again. Some definite improvements to my previous code, so thanks for that. I have made a couple of comments that I would like to discuss before merging the changes, and a test or two would definitely not hurt. How do you feel about writing the missing Tutorial 03: Frequency Correction for the documentation? I have written 1, 2, 4 and 5 and currently working on 6, but haven't gotten around to 3 yet, so if you have the time and inclination, please give it a go. Don't worry if not though, I will get there eventually.
Hi Ben! Glad to be up and running on this project after it being shut down for so long. I'd be glad to work on the frequency correction tutorial! I'm not exactly sure when that will be, but I will get to it soon.
Also - forgot to mention - I tested this on probably 25 different spectra, simple and hard cases to align, and it is good to go. The trick is to figure out a good set of bounds to use for the frequency and time domain. I'd definitely want to include some recommendations along these lines in the tutorial notebook.
| return_vector = numpy.zeros(len(residual_data) * 2) | ||
| return_vector[:len(residual_data)] = residual_data.real | ||
| return_vector[len(residual_data):] = residual_data.imag | ||
| return return_vector | ||
|
|
||
| out = scipy.optimize.leastsq(residual, initial_guess) | ||
| return out[0][0], out[0][1] | ||
| freq_and_phase_params = (out[0][0], out[0][1]) |
There was a problem hiding this comment.
this seems like an unnecessary extra line, what is the purpose of adding a variable name here and not just returning the parameters directly?
There was a problem hiding this comment.
It is definitely unnecessary - the only reason I added it is because it helps describe the meaning of the outputs a bit, and also makes the documentation a bit easier to read and understand. i.e. in the Returns section, the explanation of the outputs is simplified by capturing them in a single variable, if that makes sense....
There was a problem hiding this comment.
One thing to remember about Python is that it will automatically pack and unpack these tuples, so the existing line return out[0][0], out[0][1] is equivalent to return (out[0][0], out[0][1]) and similarly calling it like frequency, phase = spectral_registration() is the same as frequency_and_phase = spectral_registration(). If I wanted to make it really easy to read then I would probably suggest
frequency_shift = out[0][0]
phase_shift = out[0][1]
return frequency_shift, phase_shift
There was a problem hiding this comment.
I like that last suggestion, I'll update it to that. Basically, I usually try to write code that you can read - at the expense of it being less than efficient sometimes. I just felt that when I got down to the output statement, I wasn't sure which of those outputs corresponded to which parameter, and I had to go back up into the code to the place where they are applied to the data with the exponential to figure it out. So, I was just hoping to save someone else from having to do that too : )
|
|
||
| return freq_and_phase_params | ||
|
|
||
| def select_target_for_spectal_registration(data, frequency_range = None): |
There was a problem hiding this comment.
you are missing an r in spectral
There was a problem hiding this comment.
Looking at this function in detail, I am not sure whether it belongs here, or perhaps needs to be renamed. This is one way of selecting a target for spectral registration, but not the only reasonable choice. I would probably expect this function to be defined at the script level, rather than the library level, to reflect the fact that it is a particular user's preference. On the other hand, it could be renamed to more precisely describe its function, e.g. find_median_magnitude_spectrum(), which suggests it could later be accompanied by other similar functions using different criteria, whereas at the moment it comes across as a bit too prescriptive, the only possible way of selecting a target. Also it might be more appropriate for this to be a function taking a set of spectra rather than FIDs given that it is selecting based on a peak?
This is just me offering my opinion on this, very happy to have a discussion about the best way to structure things. Let me know what you think.
There was a problem hiding this comment.
I'm fine with changing the name to be more specific, or, perhaps adding an input to the function as is specifying which method to use (in this case the median method is the only one implemented), so more methods can be added over time without having to add entirely new functions. In that case, I'd probably still want to pass in the MRSData object, not the spectrum, in case future methods want to make use of the FID for some reason. What do you think?
There was a problem hiding this comment.
I would make an analogy with the spectral_registration() method - we chose not to call it do_frequency_correction() because there are other types of frequency correction which people might choose to use. By providing multiple different methods we suit different situations. Personally I think that having a set of methods is much better than the method as parameter option (do_frequency_correction(method="spectral_registration")) - it keeps both the code and the documentation simpler and makes it easier to pass different arguments to different functions, for example passing the spectrum in some cases and the FID in others.
Changed output structure for spectral registration function. Changed name of function for identifying target for spectral registration to be more explicit. Changed input to target identification function to MRSSpectrum object instead of MRSData.
Many changes made to this module
Added function for automatic identification of a target to use for spectral registration
Fixed a bug in the way the subset of the frequency range was identified
Added additional input to spectral registration function to enable limiting the time range used for alignment
Quality of spectral registration is significantly improved over previous version