smeerten / ssnake

A program for the analysis of NMR data.
Other
19 stars 15 forks source link

Phasing: Add second order phasing #148

Closed FlorianSchreiner1 closed 1 year ago

FlorianSchreiner1 commented 1 year ago

There currently is no software available to interactively correct spectra with a 2nd order phase. I implemented this functionality and made it optional to display, since it is a niche application and could potentially distract new users.

The addition of this feature is important for anyone utilizing frequency swept pulses, namely WURST pulses, because these impose a 2nd order phase on the signal. This is normally dealt with by magnitude processing the spectra, at the expense of a loss of S/N. Being able to correctly phase these spectra interactively should be a nice addition to the general workflow.

wfranssen commented 1 year ago

Hi Florian,

Thanks for the pull request. This is indeed a nice feature for ssNake to have.

The technical/back-end implementation looks fine to me. Great that you already updated the manual for your changes!

For the front-end I think we can do better. I agree with you that we must not 'clutter' the interface too much for this more rare processing option. However, by hiding it behind a setting, I am worried that users will not know it exists. (Assuming people don't read manuals.)

I would therefore propose the following:

In this way, we only add one 'row' to the phasing interface in normal operations, which I think is OK. We also show simultaneously that ssNake can do 2nd order phasing, but that it is not often needed, and therefore hidden by default.

The global setting you added can than become something like 'Always show 2nd order phasing'. So that user that use it often can get the phasing window view expanded by default.

One additional improvement over your interface implementation is that the 1st and 2nd order phasing should be in one 'frame/group'. This because the pivot point relates to both of them. (In the way you do it now, it suggests that the pivot point only influences 1st order phasing.)

The 'expanded' view in the phasing window (in the frequency domain) then becomes:

########## $ zero order # ##########

########### $ 1st order # $ 2nd order # $ pivot point # ###########

Where I attempt to show the bounding boxes with the # sign.

Let me know what you think of this, and whether you can help in implementing it.

Regards,

Wouter

FlorianSchreiner1 commented 1 year ago

Hi Wouter,

The improvements you suggested do seem like good additions to me. I should be able to implement them without much trouble.

I do like the idea of a push button to unhide the 2nd order phasing option, although I'm unsure if a checkbox below the single slice option wouldn't be better. It would allow users to hide the option again without closing the window and should visually integrate quite nicely with the single slice check box. I think it would also take up less space visually than a push button in the sense that it is removed from the 'active' part of the window.

What are your thoughts on that?

The integration of 1st and 2nd order phasing in one QGroupBox is definitely the right approach, since it does, as you pointed out, suggest the pivot point being important for both options. I think the best implementation would be to separate the individual parts into their own QWidgets which then get combined into a QGridLayout for the surrounding QGroupBox. This would also make it easy to hide and unhide them at will.

In addition to your approach of:

########## $ zero order # ##########

########### $ 1st order # $ 2nd order # $ pivot point # ###########

I would like to add a separating line between 1st and 2nd order, as well as between 2nd order and pivot point like this:

########## $ zero order # ##########

########### $ 1st order # $-------------------# $ 2nd order # $-------------------# $ pivot point # ###########

with the dashed lines being solid separators. I think this would give a bit more visual clarity to the interface, considering it is now part of the same QGroupBox.

For these changes I am very much open to your suggestions, since I don't have much experience regarding interface design.

Regards,

Florian

Edit: Corrected the markdown display.

jtrebosc commented 1 year ago

Hi Florian, I have had implemented a 2nd order phasing once: see https://github.com/jtrebosc/JTutils/blob/JTDataClass/CpyLib/JTDataClass.py). In case of 2nd order I had introduced a second pivot point where phase doesn't change when altering 2nd order phase. Phasing protocol is : -adjust 0th order phase on one spiklet

So it would be very useful to have this 2nd pivot point: A check box next to single slice adding a new section : $ 2nd order # $ pivot point # Would do the trick. Best, Julien

FlorianSchreiner1 commented 1 year ago

Hi Julien,

The implementation does seem rather simple compared to what is done now. A second point of constant phase might also make it a bit easier to actually get the spectrum phased correctly, since now you have to alternate between 1st and 2nd order quite a bit to get it right.

Regards,

Florian

wfranssen commented 1 year ago

@FlorianSchreiner1 I like your suggestions for the interface. Please go ahead :)

Regarding the suggestions by @jtrebosc: I do not really have an opinion on this. If that method is nicer for you as users for the interactive phasing, than it might indeed be better.

As Julian mentions, the interface can then just have an extra 'group' for the 2nd order phasing and its pivot point. (So in the end less changes to the interface needed than with a shared pivot point.)

Regards,

Wouter

FlorianSchreiner1 commented 1 year ago

@wfranssen Thank you! :) The second pivot point really did make it quite easy to implement the new changes.

@jtrebosc I now added the second pivot point. Because I wanted to avoid a rewrite of the actual phasing function I instead opted to do it analogously to the way the first order pivot point is implemented. Otherwise it functions exactly the same as your solution.

Regards,

Florian

jtrebosc commented 1 year ago

Dear Florian, I tested your code. Phasing seems to work as I would expect in a fairly straight forward manner. I didn't go through your code though. One thing you need to fix : the phasing buttons report an error: You added an order parameter to the phasing function. But this parameter doesn't seem to be provided at some point when phasing using the buttons (my guess from error message). Best, Julien

FlorianSchreiner1 commented 1 year ago

Hi Julien,

Sorry for committing code with blatant errors. I should have tested it better.

You were right. On one instance setRef was called without the order parameter. Additionally I used the wrong variable (self.firstVal instead of value) in two spots, leading to the push buttons not updating the first order correction. I hope I found everything wrong with the code now.

Regards,

Florian

jtrebosc commented 1 year ago

Dear Florian, It seems to work correctly indeed. This will be useful for phasing WURST CPMG experiments and possibly echo experiments using single adiabatic 180° pulses.