First draft of deconvolution revisited #181
Conversation
|
I really don't get why the CI checks are STILL failing. running black doesnt do anything, checking with flake8 shows nothing. I really tried! :( |
|
@AnnaWiniwarter CI complains about reformatting the |
|
@matenestor it changed some files before (see the various commits I made) but does nothing now: |
There was a problem hiding this comment.
Great work! I can see it's not quite finished. I've made some comments which might help. Some things will move according to the Calculator structure in #182 . Do you think it's best to finish #182 first, or this PR first?
A request: Make a demo script in the development_scripts/ folder with some okay-to-share sample data, so that I can test it out.
| #TODO:need to understand if this is actually necessary | ||
| impulse_response (ImpulseResponse): impulse response object from model/data | ||
| mol (str): molecule | ||
| F_mol (CalPoint): spectro_inlets_calibration CalPoint object |
There was a problem hiding this comment.
The data_snippet should inherit all of the original measurement's Calculators including siq or native MS calculators, so you should be able to drop this argument and instead just require that the original measurement has a Calculator for mol and use grab_flux.
| ).calc_impulse_response_from_data(tspan=tspan, tspan_bg=tspan_bg) | ||
| return impulse_response | ||
|
|
||
| def deconvolute_for_tspans( |
There was a problem hiding this comment.
This should also be a method of the Calculator (ie, the ECMSImpulseResponse.)
There was a problem hiding this comment.
leaving this unresolved since it's a not for you rather than me
|
Hi @ScottSoren, thanks for the thourough feedback! I'll get working on it. Just one reply here:
|
|
@ScottSoren Please review my revisions. :) I still need to add NEXT_CHANGES.rst |
There was a problem hiding this comment.
Hi @AnnaWiniwarter , this is a review just of the Demo script. The rest will follow.
The demo script is beautiful: reads like a tutorial. I've therefore allowed myself a bit of perfectionism in the comments, the few places where it's not quite clear, and to solicit your thoughts. Further improving it as a tutorial could be a long comment explaining that there's a "compare experimental impulse responses" section followed by a "deconvolute with modelled impulse response" section.
The one change in src that I had to make before it could run on my computer is np.trapezoid --> np.trapz. Not sure in what version trapezoid is a thing but trapz seems much more robust to me.
| impulse_response # no need to recalculate if these parameters fit | ||
| ) | ||
| else: | ||
| # re-calculate the impulse response |
There was a problem hiding this comment.
I think this will have to disappear.
There was a problem hiding this comment.
What do you mean?
There was a problem hiding this comment.
I think I hadn't understood it, but do now.
I guess it can't disappear, because you do need a kernel and t_kernel that match the timespan requested. But if you can generate a new ECMSImpulseResponse object using only the info in the previous one and the desired time vector, then kernel and t_kernel are not the defining characteristics of an ECMSImpulseResponse. There should be a method ECMSImpulseResponse.grab_kernel_for_t() that does this when called from the original object, avoiding the need to make a second one.
I'll do this when merging with [calculators].
As mentioned today, I used trapezoid as I got following deprecation warning after updating numpy (to vs 2.0.0) (Now that I realise they updated to vs 2.0.0 it makes sense that there's such a big change) Anyway, I changed it all back to |
|
@ScottSoren |
There was a problem hiding this comment.
I'll make a commit getting it to pass checks and then merge. We've left a few unresolved comments to resolve when merging with calculators (#182).
changes made by Anna from the original PR by Kevin:
*) some syntax fixes to adjust for changes made to ixdat overall (up to 0.2.8)
*) remove the class DecoMeasurement and put the related methods directly into the ECMSMeasurement class. renamed some of the methods so they are more aligned with ixdat conventions
*) Change the name "Kernel" to "ECMSImpulseResponse". Might be the correct mathematical term, but I found it confusing (especially since Python also has Kernels)
*) Tried to remove the differentiation between whether an "ECMSImpulseResponse" is from a measurement or modelled, as they are both essentially just a tuple of (time, value). I think some is still left
There are some unfinished methods, which are not priority for me at the minute, but I'm not sure if it's necessary to wait for them to be finished and polished before merging.
Eventually, 'deconvolution' will be one 'modifier' (or how did we call it?) among many, but I wanted to get this running and available for others earlier than this major overhaul.
What do you think @ScottSoren?
I know that NEXT_CHANGES.rst is still missing - will provide if merge makes sense already.