Skip to content
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

BUG - Copy isotopic detail to clipboard doesn't copy EIC when correct for natural abundance is enabled #468

Open
lparsons opened this issue Nov 22, 2017 · 20 comments
Assignees

Comments

@lparsons
Copy link
Contributor

When the "Correct for Natual C13 abundance" option is enabled, copying the isotopic detail only copies the fraction instead of the EIC.

@lparsons lparsons changed the title BUB - Copy isotopic detail to clipboard doesn't copy EIC when correct for natural abundance is enabled BUG - Copy isotopic detail to clipboard doesn't copy EIC when correct for natural abundance is enabled Nov 22, 2017
@shubhra-agrawal
Copy link
Collaborator

shubhra-agrawal commented Dec 16, 2017

@lparsons Can you list out the steps to reproduce this bug? Primarily, which clipboard functionality you are referring to? The ones I am aware of are:

  1. Copy EIC image to clipboard
  2. Copy Group Information to Clipboard
  3. Copy EIC(s) to Clipboard (right-clicking in the EIC window)
  4. Copy to Clipboard (right-clicking a group in the bookmark or peaks table)

I tried the above options with the Natural abundance correction enabled/disabled and the results match. Maybe I'm missing something.

Edit: It seems I did not use enough examples the first time. I see the PeakArea values are entirely different when NA correction is enabled. Looking into it.

shubhra-agrawal added a commit that referenced this issue Dec 19, 2017
Correct for natural C13 abundance has not been validated in El-MAVEN. Disabling the option by default till this is fixed.

Issue: #468
shubhra-agrawal added a commit that referenced this issue Dec 19, 2017
NA correction is returning fractions instead of corrected PeakArea values.
The fractions are useful for Isotope plot (needs validation) but not for clipboard functionality.
This commit will:
- retain the same isotope plot behaviour
- disable NA correction for clipboard functionality i.e. show uncorrected PeakArea values

Issue: #468
shubhra-agrawal added a commit that referenced this issue Dec 19, 2017
NA correction has some bugs and has not been validated.
User discretion recommended.

Issue: #468
@lparsons
Copy link
Contributor Author

lparsons commented Dec 19, 2017

I've contacted the person here who originally reported this, but have not heard back. I'm going to see what I can do to try and reproduce it myself. Will update with my findings.

UPDATE - I see you've been able to reproduce. Thanks. Let me know if I can help with anything else.

shubhra-agrawal added a commit that referenced this issue Dec 21, 2017
When NA correction is enabled, 'copy to clipboard' copies NA fractions instead of Peak values.
This commit will copy both NA fractions and Peak values when NA correction is enabled.
shubhra-agrawal added a commit that referenced this issue Dec 21, 2017
Using the clipboard functionality, NA values are shown along with intensities. If NA correction is disabled, it shows 0 value for NA fractions.
This commit will ensure that NA abundance matrix is only copied when report isotopes and NA correction is enable.

Issue: #468
sahil21 pushed a commit that referenced this issue Jan 3, 2018
When NA correction is enabled, 'copy to clipboard' copies NA fractions instead of Peak values.
This commit will copy both NA fractions and Peak values when NA correction is enabled.
sahil21 pushed a commit that referenced this issue Jan 3, 2018
Using the clipboard functionality, NA values are shown along with intensities. If NA correction is disabled, it shows 0 value for NA fractions.
This commit will ensure that NA abundance matrix is only copied when report isotopes and NA correction is enable.

Issue: #468
@shubhra-agrawal
Copy link
Collaborator

shubhra-agrawal commented Jan 4, 2018

@lparsons If NA correction is enabled in El-MAVEN, copy to clipboard will provide both AreaTop(or Area or Height as selected from the quantitation box) and NA fractions.
If NA correction is disabled, only intensity information will be provided.

I have also unchecked NA correction by default as the algorithm has not been validated.

This fix has been merged to develop so you can test it out and suggest changes to the format in which this data is copied.

@lparsons
Copy link
Contributor Author

I believe that the peak quantification values are correct now when NA correction is on or off. I presume they will respect the quantitation box settings (i.e. output AreaTop, Height, etc.) regardless of NA correction.

However, I'm not sure that the NA fractions are correct (at least they look pretty suspicious to me). See example below:

Sample F-1-1 F-1-2 F-1-3 F-2-1 F-2-2 F-2-3
C16:0 | C12 PARENT 42453104 35224488 44299132 35620892 34871812 35850468
C16:0 | C13-label-1 0 6452194.5 7766729.5 6433422 6114256 6479434.5
C16:0 | C13-label-10 6205314.5 5023870 6852986.5 4956412.5 4952998 5429161.5
C16:0 | C13-label-11 3993003 3152660.75 4430734.5 4184492.75 4179464.25 4382364
C16:0 | C13-label-12 18254480 14822297 20526896 15376087 15260389 16415796
C16:0 | C13-label-13 9066036 7172865.5 9540149 9329944 9352811 9921999
C16:0 | C13-label-14 36029716 29523200 39533108 28984394 29739336 31598378
C16:0 | C13-label-15 9644588 7982493.5 10600837 9089420 9648703 9983346
C16:0 | C13-label-16 33621824 27805590 36793276 25396742 26448312 26912154
C16:0 | C13-label-2 1243447.5 985047.69 1372776.88 985525.31 992734.81 1007780
C16:0 | C13-label-3 125412.88 113873.85 140713.25 126851.1 116158.78 107396.25
C16:0 | C13-label-4 164646.3 131235.28 186878.94 55292.23 36909.19 66666.31
C16:0 | C13-label-5 44565.62 67753.01 69648.57 0 36820.79 26965.54
C16:0 | C13-label-6 0 368901.12 525816.75 252126.19 212205.12 292127.66
C16:0 | C13-label-7 268779.31 239701.3 272747.97 227911.89 216057.05 250682.42
C16:0 | C13-label-8 1614449.38 1496671.38 1903616.12 1188624.5 1098693.12 1232686.38
C16:0 | C13-label-9 0 899949 1196330.62 1130307 1115718.88 1266399.12
Natural Abundance            
C16:0 | C12 PARENT 0.31 0.3 0.28 0.3 0.29 0.28
C16:0 | C13-label-1 0 0 0 0 0 0
C16:0 | C13-label-10 0.04 0.04 0.04 0.04 0.04 0.04
C16:0 | C13-label-11 0.02 0.02 0.02 0.03 0.03 0.03
C16:0 | C13-label-12 0.12 0.12 0.12 0.12 0.12 0.12
C16:0 | C13-label-13 0.05 0.04 0.04 0.06 0.06 0.06
C16:0 | C13-label-14 0.24 0.23 0.23 0.22 0.22 0.23
C16:0 | C13-label-15 0.04 0.04 0.04 0.05 0.05 0.05
C16:0 | C13-label-16 0.22 0.21 0.21 0.19 0.19 0.19
C16:0 | C13-label-2 0 0 0 0 0 0
C16:0 | C13-label-3 0 0 0 0 0 0
C16:0 | C13-label-4 0 0 0 0 0 0
C16:0 | C13-label-5 0 0 0 0 0 0
C16:0 | C13-label-6 0 0 0 0 0 0
C16:0 | C13-label-7 0 0 0 0 0 0
C16:0 | C13-label-8 0 0 0 0 0 0
C16:0 | C13-label-9 0 0 0 0 0 0

@sp-eldata
Copy link
Member

@lparsons We would suggest looking at IsoCorrect on Polly to be able to do this. We are not supporting NA Correction within El-Maven anymore. Polly has a single click app to do the same with El-Maven data.
Soon, we will also be introducing closer connections between Polly and El-Maven as well.

@lparsons
Copy link
Contributor Author

lparsons commented Jan 31, 2018

We are not supporting NA Correction within El-Maven anymore.

@sp-eldata This is a bit concerning to me for two reasons:

  1. If an option/feature is no longer supported and not working properly, I strongly believe that it should be removed from the software. Having it there, but not function properly leads to confusion and a lack of trust by the researchers who are using the software.
  2. I'm concerned that features are being "removed", or at least not implemented, in ElMaven (which is open source), in favor of implementation in Polly. It was my understanding that algorithms would be implemented in a base library and then used by both ElMaven (open source for "power users"), and Polly (closed source, but easy to use). @shekhjha Do you have any comment on this?

@sp-eldata
Copy link
Member

sp-eldata commented Jan 31, 2018

@lparsons Thanks for raising the concerns.
1 - Agreed. But we wouldn't want to 'remove' anything without checking with the users well in advance.

2 - The core of El-Maven will remain open source and available freely forever. We believe that as an application trying to do NA Correction also in El-Maven is asking too much from a single application. It's focus should remain towards processing raw mass spec data. We will keep supporting core El-Maven functions openly.
That said, the Python package behind NA Correction on Polly will also be made open source. Engineering will remain closed sourced. The science will be open source.

Does that address your concerns?

@chubukov
Copy link
Collaborator

@sp-eldata @lparsons I agree completely with Lance's comments above.

I'm not sure it's the best option, but there may be some middle of the road solution here -- as far as I remember, historically Maven has only corrected for natural 13C. So you could maintain that functionality in Maven while still having more advanced options available in Polly.

@lparsons
Copy link
Contributor Author

@sp-eldata Thanks for the thoughtful reply.

  1. I understand that you don't want to pull the rug out from underneath users by removing the feature. However, I think it needs to work properly if it there, even if it's "deprecated".
  2. I agree, that stating that this function is "out of scope" for ElMaven is reasonable. I'm also happy to hear that "science is open source, engineering is closed source". That's certainly a stance I can understand.

Finally, I agree with @chubukov that a middle ground solution would be useful. I'd say that the natural 13C functionality could be kept, but perhaps move the option to a "deprecated" pane, and use that to point users to Polly? Regardless, if it's there, it should work.

@sp-eldata
Copy link
Member

@lparsons We do want to move NA Correction to the 'deprecated' track. That will also mean that at some point we should stop developing and fixing bugs etc right?

@chubukov The question is whether correction for natural 13C is something that El-Maven should do. Does it fall within it's scope as an application? Happy to hear your thoughts.

@lparsons
Copy link
Contributor Author

@sp-eldata Sure, I would say then, to close this issue either move the NA Correction to a "deprecated" tab in the options dialog (or somehow clearly indicate that it no longer works) or fix the known bugs at leave it as is.

@chubukov
Copy link
Collaborator

@sp-eldata I think the point is to not have any deprecated options masquerading as real options. In our group we "know" that it doesn't always work, so we don't try to use it. That wouldn't be clear to anyone else. So it should probably just be eliminated as an option now if it's not being tested/supported.

As to whether correction for natural 13C should be within the scope of El-Maven, I could go either way. Historically, it was there (it did work to some extent at least). It's clearly useful. And it's clearly doable to implement it well within Maven. Is it absolutely required for function? Of course not.

@shekhjha
Copy link

@chubukov @lparsons We agree. I will try to paraphrase some of the points that have already been addressed.
Correction for natural abundance is currently one of the many features that do not work as expected. We continue to fix them as we go along. At the least, making sure the user gets clear message in cases where a particular feature does not work is also something that is better than before. Clearly, a lot of ground needs to be covered. We expect participation from the community of developers to join us to do that as well. A lot of these issues have been documented just here.

Specifically, support for 13C correction can be fixed in the short term. Personally, I never used it but I understand it would be part of many user's workflows. More importantly, we believe, use cases of using other labels (2H, 15N etc), double labels, is beyond the scope of a tool like El-MAVEN. It should be focused on MS data processing alone. We have built a python package (corna) to address a number of such use cases. This effort has been primarily driven by @raaisakuk with a lot of help from @chubukov. This package will be open sourced as well. Currently, the accompanying manuscript is under preparation.

@lparsons
Copy link
Contributor Author

@shekhjha Thanks for the update. I think my main point is that this issue shouldn't be closed just because the feature is being deprecated. There should be a decision made about it's inclusion, and then action taken. It sounds like that is the general consensus.

I'd like to have a discussion about the these scope issues with some folks from the lab here, as well as get a better idea of the tools being developed. Looking forward to seeing more about the python package, etc. Finding the right tool for the job is an important part of bioinformatics and research.

@lparsons
Copy link
Contributor Author

I spoke with @LiChenPU from our lab and he agrees that removing the NA correction would be fine. Should we create a new issue to move that function to a "deprecated" branch and then close this as "won't fix"?

Also, as an aside, we have some NA correction code in R that we can use as a post-processing step. Any advice on connecting external functions with ElMaven?

@sp-eldata
Copy link
Member

@lparsons @chubukov So as a consensus, we will move this to a deprecated branch. And start giving some feedback to the user to indicate that it is deprecated.

Thanks for the tough questions. They are essential to make sure that we are making the right choices for the community. Please lmk if there is any more confusion around this.

@sp-eldata
Copy link
Member

@lparsons Couple of ways to connect R code:

  • Bundle it as executable. This is what we have done for alignment. Alignment code is in Python.
  • You could also add R executable from the options dialogue. We are not sure if this is working though. We haven't tried it anytime recently.
    screen shot 2018-02-01 at 10 01 48 am

@chubukov
Copy link
Collaborator

chubukov commented Feb 1, 2018

@sp-eldata the built-in R support was one of the things we decided to deprecate fairly early on, so I very much doubt it's working properly. It had fairly limited functionality to begin with though (in terms of access to the data). It would have to be rewritten to handle all the new data structures.

@lparsons are you just asking what the best way is to integrate El-Maven output with your R package? I would suggest exporting as CSV (either long or summary form, up to you), and then you can do whatever processing you want with your R package and keep track of both corrected and uncorrected data.

@lparsons
Copy link
Contributor Author

lparsons commented Feb 1, 2018

@sp-eldata I'm not sure what you mean by "move this to a deprecated branch", but yes, making it clear to the user that the option is no longer reliable is very important, imho. My suggestion would be to create a pane of options that are "deprecated", add some text to that options pane to explain it, and make sure those options are all "off" by default.

@chubukov I agree that some simple loose-coupling is a better idea in general, but there is strong desire from the people in the lab to make the analysis workflow simple. That being said, I think we can figure out some way to connect the two things that is relatively simple for the users. People here have reported using the built-in R support and were asking about it. Another candidate to move the deprecated options pane.

Would people be in favor of creating an issue to capture the various things we want to deprecate and start getting those options moved in the UI? I'm happy to get that started (and perhaps do some actual coding work if I can scrape up some time ;-) ).

@shubhra-agrawal
Copy link
Collaborator

@lparsons @chubukov @sp-eldata @shekhjha I've created a new issue #586 for this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants