Skip to content

Nmrpca fix egligibility - #448

Merged
comcon1 merged 7 commits into
NMRLipids:mainfrom
comcon1:nmrpca-fix-egligibility
Feb 18, 2026
Merged

Nmrpca fix egligibility#448
comcon1 merged 7 commits into
NMRLipids:mainfrom
comcon1:nmrpca-fix-egligibility

Conversation

@comcon1

@comcon1 comcon1 commented Jan 23, 2026

Copy link
Copy Markdown
Member

Now, all the lipids having sn-1 and sn-2 tails are egligible. List is not hardcoded anymore.


📚 Documentation preview 📚: https://databank--448.org.readthedocs.build/

@comcon1
comcon1 requested a review from pbuslaev January 23, 2026 16:01
@comcon1
comcon1 force-pushed the nmrpca-fix-egligibility branch 2 times, most recently from d6aa336 to 9beb4b9 Compare January 27, 2026 13:14
def verify_lipid(lipid: Lipid) -> bool:
"""Verify that the lipid is supported by the method."""
__tailsn1_vars = ["sn-1", "sn1"]
__tailsn2_vars = ["sn-2", "sn2"]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should this be stored elsewhere and only imported here? I believe that tail-names are universal and not class-specific.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also, I believe that this method can hardly be generalized when 4-tailed lipids become available. Should the Parser class have a dict of supported tail combinations

from collections import Counter
from ... import TAIL_VARS, REVERSE_TAIL_VARS  # dictionary of tail alt-names
SUPPORTED_LIPIDS = {
    2: ["sn1", "sn2"],  # List of tail names for lipids with 2 tails
}

@classmethod
def verify_lipid(cls, lipid: Lipid) -> bool:
    tails = []
    for v in lipid.mapping_dict.values():
        if v["FRAGMENT"] in REVERSE_TAIL_VARS:
            tails.append(REVERSE_TAIL_VARS[v["FRAGMENT"]])
    if len(tails) not in SUPPORTED_LIPIDS:
        return False
    return Counter(tails) == Counter(SUPPORTED_LIPIDS[len(tails)])
"""

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I isolated the function of counting tails because it should not belong to this module. Tail naming should be documented for various types of lipids. sn-1/2 is only for glycerolipids of non-archea. It doesnt' cover the variability. Will develop the function tomorrow according to which tests I will invent.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've made. Not exactly this way, but something like

@comcon1
comcon1 force-pushed the nmrpca-fix-egligibility branch from 996c870 to e95a061 Compare February 5, 2026 09:51
@comcon1
comcon1 requested a review from pbuslaev February 5, 2026 13:04
@comcon1
comcon1 force-pushed the nmrpca-fix-egligibility branch 2 times, most recently from 651a2e7 to 1ce730a Compare February 11, 2026 09:09
@comcon1
comcon1 force-pushed the nmrpca-fix-egligibility branch from 1ce730a to 3abaeb2 Compare February 11, 2026 09:19
@comcon1

comcon1 commented Feb 11, 2026

Copy link
Copy Markdown
Member Author

We delivered quite a large PR in the mean-while, so I rebased, force-pushed and verified integration tests.


HEADGRP = "headgroup" # locally used to gather amber lipids together
TAILSN1 = "sn-1"
TAILSN2 = "sn-2"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@comcon1 As you've mentioned, this is very specific for Amber and two tails. Should we add a NOTE or TODO to mark this bit to improve in the future?

@pbuslaev pbuslaev left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All looks really great. There is a small comment about adding a NOTE of TODO or whichever mark, but rather than that, I am happy with this change

@comcon1
comcon1 merged commit 2046184 into NMRLipids:main Feb 18, 2026
10 of 11 checks passed
@comcon1
comcon1 deleted the nmrpca-fix-egligibility branch February 20, 2026 08:17
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.

2 participants