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

Expose ImzMLParser.polarity, fix ReadTheDocs #27

Merged
merged 6 commits into from
Jul 19, 2021

Conversation

LachlanStuart
Copy link
Contributor

Several changes to make my life easier when working with ImzML files:

  • Added a ImzMLParser.polarity field calculated from the header ReferenceableParamGroups and first spectrum metadata
    • Also added a new fix to the ontology processing to handle incorrect accession="MS:1000128" name="positive scan" params.
    • I mainly added this because the imzml tools and WIP recalibration code don't currently write polarity information in the output ImzML, because it was difficult to get it from ImzMLParser consistently. Without polarity information, some other ImzML tools (e.g. MSIReader) won't load the processed files.
  • Added a patch to fix the polarity accession code when exporters incorrectly use MS:1000128 ("profile spectrum") instead of MS:1000130 ("positive scan"). I checked all the ImzML headers from METASPACE (as of ~8 months ago) and found that when this incorrect accession is used, it's always in the referenceableParamGroups in the header, so this doesn't need any fix in the per-spectrum metadata reading code.
  • Changed the default parser to ElementTree. lxml was originally used because it's several times faster, but it has a tendency to segfault the Python process on larger ImzML files, at least on Linux. This can be pretty frustrating and is really hard to figure out in environments that aren't so sensitive to the Python process abruptly ending (e.g. Jupyter Notebooks & PyCharm's console), so to avoid further frustration I changed the default to be the safer implementation.

Also, as this library has quite a few external users now, I've been working to try to improve its overall quality:

  • Added a CHANGELOG.md
  • Tried to fix the documentation, as it's missing a lot. I also turned on the webhook for ReadTheDocs, so this should auto-update once it's merged. I copied some of the patterns from METASPACE's python-client

Copy link
Contributor

@sergii-mamedov sergii-mamedov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -90,7 +91,7 @@

# List of patterns, relative to source directory, that match files and
# directories to ignore when looking for source files.
exclude_patterns = ['_build']
exclude_patterns = ['_build', 'tests', 'setup']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems necessary to rename _build to build

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I just tested building locally and found that actually this exclude_patterns value isn't needed because I disabled the automatic .rst-file generation by removing rtd_gen_docs, and the other .rst files specify exact modules paths.

I changed it to exclude the whole docs directory, just in case the automatic module-finding is turned on again in the future.

@LachlanStuart LachlanStuart merged commit b5162bf into master Jul 19, 2021
@LachlanStuart LachlanStuart deleted the feat/parser-polarity branch July 19, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants