add pandas df conversion by axelwalter · Pull Request #5622 · OpenMS/OpenMS

axelwalter · 2021-10-28T13:59:25Z

Description

Add conversion for FeatureMap, ConsensusMap, MSExperiment and list of PeptideIdentification.

Checklist:

Make sure that you are listed in the AUTHORS file
Add relevant changes and new features to the CHANGELOG file
I have commented my code, particularly in hard-to-understand areas
New and existing unit tests pass locally with my changes
Updated or added python bindings for changed or new classes. (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI:

If your PR is failing you can check out

http://cdash.openms.de/index.php?project=OpenMS and look for your PR. If you click in the column that lists the failed tests you will get detailed error messages.

Note:

Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

jpfeuffer

Cool! A few changes would be nice.
And have a look at CodeFactor.

jpfeuffer · 2021-10-28T14:11:14Z

 from . import Plotting as Plot

+try:
+  from .Dataframes import ConsensusMap, FeatureMap, MSExperiment, DFConverter


To be honest, I don't think we should import this subpackage by default.

OpenMS/src/pyOpenMS/pyopenms/__init__.py

Line 59 in 0a9f260

from .python_extras import *

IMHO users should do from pyopenms import Dataframes if they want that functionality, and only then it should try to import pandas.

I have no idea though if we would need to hide core classes under a separate subnamespace (E.g. core) or what happens if you do from pyopenms import *.
Needs to be tested.

Maybe also namespaces should be lowercase? So you don't confuse with classes?

To be honest, I don't think we should import this subpackage by default.

OpenMS/src/pyOpenMS/pyopenms/__init__.py

Line 59 in 0a9f260

from .python_extras import *

IMHO users should do from pyopenms import Dataframes if they want that functionality, and only then it should try to import pandas.

Hmm what would be the downside if we make pandas a requirement for pyOpenMS and by default include the Dataframes as Axel did?

I kind of would like to make this as easy as possible for the user

Downside is that people always need pandas.
And it would need to go into the setup.py

I am in the camp of "batteries included" and we already require numpy, so I think requiring pandas is not that big of a step.

OK. Fine with me. I guess then we can leave those imports and just need to add it to setup.py.
And install it on the build slaves.

jpfeuffer · 2021-10-28T14:14:24Z

+    b'score_fit': 'f',
+    b'score_correlation': 'f',
+    b'FWHM': 'f',
+    b'spectrum_native_id': 'U30',


Think that's enough?

hroest

Can you please add tests for this new functionality?

jpfeuffer · 2021-10-28T15:59:06Z

@hroest Regarding tests, when should we load pandas? What is your opinion?

E.g.:

check during cmake already?
check in setup.py?
check in nose/test/script?

hroest · 2021-10-28T16:20:35Z

If its required to run the tests, then I would say we should check in cmake.

timosachsenberg · 2021-10-28T16:46:19Z

What is your opinion if someone takes a shortcut and does testing via the jupyter notebooks? At least I had the impression that people are more willing to write tests by creating an example in the readthedocs page. It now get's automatically tested. i.e., we get a good test coverage. Of course, the "right thing to do" is to do unit testing in the OpenMS repo.

hroest · 2021-10-29T02:50:40Z

What is your opinion if someone takes a shortcut and does testing via the jupyter notebooks?

do we check that these still work with every change in OpenMS? I see that happening with the documentation that requires updating for a new release...

timosachsenberg · 2021-10-29T05:01:08Z

No I think Julianus added that it only tests the changed notebooks. Initially it tested all of manually triggered or at least one notebook changed

timosachsenberg · 2021-10-29T05:03:09Z

@jpfeuffer would or make sense to regularly test all e.g. nightly?

jpfeuffer · 2021-10-29T07:49:14Z

Sure, just send a workflow dispatch trigger from the pyopenms builds to the "push action" of the binder branch.
https://github.community/t/triggering-by-other-repository/16163

timosachsenberg · 2021-10-29T07:50:50Z

Sure, just send a workflow dispatch trigger from the pyopenms builds to the "push action" of the binder branch. https://github.community/t/triggering-by-other-repository/16163

and create a new action for considering all files?

jpfeuffer · 2021-10-29T07:51:38Z

It's there already. Just never gets triggered. See issues.

jpfeuffer · 2021-10-29T07:53:19Z

But to be honest, since you have to add pandas in OpenMS now anyway, we can also just add a one liner to the tests to at least see that it works.
Maybe test the length of the dataframe.

timosachsenberg · 2021-10-29T07:54:54Z

so how would that look like:
OpenMS get'a push -> trigger pyopenms and whl need to be build via pyopenms-extra? -> trigger the test action

jpfeuffer · 2021-10-29T07:55:43Z

Yes but currently pyopenms only runs on pushes to nightly.

timosachsenberg · 2021-10-29T07:55:55Z

But to be honest, since you have to add pandas in OpenMS now anyway, we can also just add a one liner to the tests to at least see that it works. Maybe test the length of the dataframe.

sure it is the better way to do it. still would be great to include the new documentation and example notebooks into the testing (e.g. only nightly)

jpfeuffer · 2021-10-29T07:57:17Z

Problem is that pyopenms builds take 3.5 hours.

https://stackoverflow.com/questions/47272460/python-tuple-unpacking-in-return-statement

jpfeuffer · 2021-11-02T15:34:48Z

Hmm wasn't enough.

jpfeuffer · 2021-11-02T18:48:09Z

Feel free to add tests now. @timosachsenberg @axelwalter

timosachsenberg · 2021-11-03T07:34:13Z

I think we can add code from the pyOpenMS examples on the readthedocs page here that also tests writing and reading of files as well as conversion of the resulting maps to pandas:

for featureXML
https://github.com/OpenMS/OpenMS/blob/develop/src/pyOpenMS/tests/unittests/test000.py#L2098
for consensusXML
https://github.com/OpenMS/OpenMS/blob/develop/src/pyOpenMS/tests/unittests/test000.py#L873
for mzML file
https://github.com/OpenMS/OpenMS/blob/develop/src/pyOpenMS/tests/unittests/test000.py#L3662
and idXML:
https://github.com/OpenMS/OpenMS/blob/develop/src/pyOpenMS/tests/unittests/test000.py#L2209

axelwalter · 2021-11-03T09:19:49Z

I will work on a readthedocs tutorial and add the examples as test cases.

jpfeuffer · 2021-11-03T10:31:21Z

Hmm but wouldn't it be better to have it merged first with a very basic test?

timosachsenberg · 2021-11-03T10:52:28Z

yep I agree.e.g. just adding one or two features and then test the df shape

…lter/OpenMS into feature/pandas-dataframes

jpfeuffer · 2021-11-04T15:42:00Z

I added quite some Todos that should be addressed in the future, in addition to an idXML test. But from my side we could merge it after CI.

…lter/OpenMS into feature/pandas-dataframes

timosachsenberg

Feel free to merge if tests pass

axelwalter · 2021-11-08T09:13:11Z

Still need to fix handling missing values in meta values.

jpfeuffer · 2021-11-09T10:13:18Z

    switchDict = {bool: '?', int: 'i', float: 'f', str: 'U100'}
+
+    # filter out PeptideIdentifications without PeptideHits if export_unidentified == False
+    if not export_unidentified:


Thanks for taking my suggestion, however, I would do it during the main iteration.
In C++ this would be fine, but in python this will do copies of each PeptideID.
I make a comment on where I would add it.

EDIT: Ah you might need the count, right? Maybe just use this pre-iteration for the count then.
EDIT2: Hmm maybe does not matter. Since we do not have a count function, we need to copy anyway.

First I put it in the main iteration (line 302), however, returning None (when there are no peptide hits and export_unidentified == False) does not work with np.fromiter(). Therefore I filtered them out in advance.

In this case, I would just "continue" the loop without yielding, but I think we can leave it for now. Maybe add a TODO.

Sorry, I might have gotten it wrong. Yes we would need to think about it.

Ok. Is it possible to continue here since we pass only one PeptideIdentification at a time and it is not really a loop?

I just checked and yes you can just do "return" without argument.

I added a commit

Not sure if the iteration for counting is worth it, but we have to benchmark.

timosachsenberg · 2021-11-09T12:10:16Z

woohhoo!

timosachsenberg · 2021-11-10T10:44:37Z

I think peptide evidences are not exported (e.g, the protein accessions and start, end in the protein)

add pandas df conversion

e1b2457

axelwalter requested review from jpfeuffer and timosachsenberg October 28, 2021 14:00

jpfeuffer added the NoJenkins Skips Jenkins tests for PRs label Oct 28, 2021

jpfeuffer requested changes Oct 28, 2021

View reviewed changes

hroest self-requested a review October 28, 2021 15:07

hroest requested changes Oct 28, 2021

View reviewed changes

axelwalter added 3 commits October 29, 2021 09:22

fix codefactor error: else after return

91bd3c1

remove TODO comments

36c6dd5

import modules from .

359fab9

axelwalter and others added 6 commits October 29, 2021 10:53

use get2DPeakDataLong() for MSExperiment DF

6be4d69

rename dataframes module to lower case

feb6ba8

add pandas to requirements

c4af1f0

unpacking directly in return/yield only allowed from py3.8

7fae880

https://stackoverflow.com/questions/47272460/python-tuple-unpacking-in-return-statement

Add ability to use copy constructors

e396b71

Also set the typedef PeakMap to the new subclass

70e3ccf

jpfeuffer added 2 commits November 2, 2021 16:45

Import peakmap to override it

5733348

Also import PeakMap in python_extras

b14a473

jpfeuffer and others added 4 commits November 4, 2021 12:15

Add cxml test

bc5a38a

add featuremap test

e796a2e

Merge branch 'feature/pandas-dataframes' of https://github.com/axelwa…

631d179

…lter/OpenMS into feature/pandas-dataframes

[DOC] update documentation

ec458fb

axelwalter added 3 commits November 4, 2021 17:04

add MSExperiment().get_df() test

e5001e0

Merge remote-tracking branch 'origin' into feature/pandas-dataframes

050ecb9

Merge branch 'feature/pandas-dataframes' of https://github.com/axelwa…

21a6c04

…lter/OpenMS into feature/pandas-dataframes

timosachsenberg approved these changes Nov 6, 2021

View reviewed changes

axelwalter added 2 commits November 8, 2021 09:24

missing charge values to pandas.NA

6bab1a6

add test for PeptideIdentifications to df

72eb3f7

fix missing values for peptide identification meta values

c5e7cfb

jpfeuffer reviewed Nov 9, 2021

View reviewed changes

axelwalter and others added 2 commits November 9, 2021 11:31

add TODO for filtering PeptideHits in main iteration

8377fc7

skip unidentified in main iteration

8ecb4aa

jpfeuffer merged commit 84d15f1 into OpenMS:develop Nov 9, 2021

axelwalter deleted the feature/pandas-dataframes branch March 17, 2022 08:43

Uh oh!

Uh oh!

Conversation

axelwalter commented Oct 28, 2021

Description

Checklist:

How can I get additional information on failed tests during CI:

Note:

Uh oh!

jpfeuffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hroest left a comment

Choose a reason for hiding this comment

Uh oh!

jpfeuffer commented Oct 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hroest commented Oct 28, 2021

Uh oh!

timosachsenberg commented Oct 28, 2021

Uh oh!

hroest commented Oct 29, 2021

Uh oh!

timosachsenberg commented Oct 29, 2021

Uh oh!

timosachsenberg commented Oct 29, 2021

Uh oh!

jpfeuffer commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timosachsenberg commented Oct 29, 2021

Uh oh!

jpfeuffer commented Oct 29, 2021

Uh oh!

jpfeuffer commented Oct 29, 2021

Uh oh!

timosachsenberg commented Oct 29, 2021

Uh oh!

jpfeuffer commented Oct 29, 2021

Uh oh!

timosachsenberg commented Oct 29, 2021

Uh oh!

jpfeuffer commented Oct 29, 2021

Uh oh!

jpfeuffer commented Nov 2, 2021

Uh oh!

jpfeuffer commented Nov 2, 2021

Uh oh!

timosachsenberg commented Nov 3, 2021

Uh oh!

axelwalter commented Nov 3, 2021

Uh oh!

jpfeuffer commented Nov 3, 2021

Uh oh!

timosachsenberg commented Nov 3, 2021

Uh oh!

jpfeuffer commented Nov 4, 2021

Uh oh!

timosachsenberg left a comment

jpfeuffer commented Oct 28, 2021 •

edited

Loading

jpfeuffer commented Oct 29, 2021 •

edited

Loading

jpfeuffer Nov 9, 2021 •

edited

Loading

jpfeuffer Nov 9, 2021 •

edited

Loading