Complete HGVS object handling cleanup in mappers.py and hgvs_utils.py

[Peter-J-Freeman]

• replace regex and string-based intronic position checks with HGVS position helpers
• use structured HGVS edit types instead of formatted-string inspection
• reduce unnecessary HGVS object stringification and reparsing
• improve direct HGVS object construction and preserve position metadata
• fix duplicated start-offset checks to correctly inspect start and end positions
• support structured intronic checks across CDS and UTR coordinates

[Peter-J-Freeman]

@John-F-Wagstaff This is not to be merged yet, it is just to get a review of the structure and the 2 filed completed so far. I cannot say they are totally regex and string object free, but I think they should be close

[codecov]

Codecov Report

❌ Patch coverage is 86.99764% with 385 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.84%. Comparing base (e2036de) to head (18251b0).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
VariantFormatter/variantformatter.py	81.79%	79 Missing ⚠️
VariantValidator/modules/vvMixinCore.py	73.78%	70 Missing ⚠️
VariantValidator/modules/hgvs_utils.py	85.82%	37 Missing ⚠️
VariantFormatter/gapGenes.py	78.66%	32 Missing ⚠️
VariantFormatter/formatter.py	85.57%	29 Missing ⚠️
VariantValidator/modules/complex_descriptions.py	89.75%	25 Missing ⚠️
VariantValidator/modules/liftover.py	87.86%	25 Missing ⚠️
VariantValidator/modules/vvDatabase.py	87.01%	20 Missing ⚠️
VariantFormatter/simpleVariantFormatter.py	88.51%	17 Missing ⚠️
VariantValidator/modules/valoutput.py	88.99%	12 Missing ⚠️
... and 11 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #870      +/-   ##
===========================================
+ Coverage    82.51%   84.84%   +2.32%     
===========================================
  Files           42       48       +6     
  Lines        13913    14554     +641     
===========================================
+ Hits         11481    12348     +867     
+ Misses        2432     2206     -226     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[John-F-Wagstaff]

if we are going to have to specify what we want in the args anyway then, given both the readability issues and the fact that each usage point will only ever use the one version, it would probably be better to have a set of functions, something like:
pos_is_intronic # (or pos_any_intronic for any intronic end
pos_fully_intronic # for both
pos_start_intronic # for just start
and
pos_end_intronic # for just end

each of these would be close to one line functions but this would improve the readability of the usage sites and even remove at least 1 if/else check as well.

[Peter-J-Freeman]

so, split all into sub functions? even the UTR ones. I will do this now while I am refactoring a couple more scripts if I understand correctly

1 finction = 1 posiiton.

I think we lose both. Prob not needed

[John-F-Wagstaff]

We probably still want the both and either options, because it simplifies the if statements.

Overly complex if statements are one of the most annoying things we have to put up with from a readability perspective. Having the both/either version also means that we only have to run is_objet once rather than twice when we do use them, which is a small but repeatable performance gain.

[John-F-Wagstaff]

For the UTR functions I am not so certain, we do use this less. Also one of the two, start/end is functionally equivalent to checking for fully 3'/5/ UTR depending on which UTR end we are using. It may be worth checking the logic in the call sites and considering a fully UTR/partially UTR/any UTR versions of these functions, as opposed to the current start and end versions.

We may also want UTR non end specific helper functions, if the question is "does this affect the CDS" then this should translate to "is this fully UTR", regardless of the end.

Unfortunately I am less certain here. I have paid less attention to the UTR detection code than the intronic/exonic code in the past, due to the alignment issues I have ended up looking at being focused on the intron-exon boundaries.

[Peter-J-Freeman]

may be overkill

for now I am going to add

def is_object(hgvs_object):
    """
    Check that the supplied value is a parsed HGVS object.

    Parameters
    ----------
    hgvs_object
        Object expected to be a parsed HGVS SequenceVariant.

    Raises
    ------
    HgvsPositionException
        If the supplied value does not have an HGVS accession attribute.
    """
    try:
        hgvs_object.ac
    except AttributeError:
        raise HgvsPositionException(
            f"Variant {hgvs_object} is not a parsed hgvs object"
        )


def start_position_is_intronic(hgvs_object):
    """
    Return whether the start position is intronic.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.start.offset != 0


def end_position_is_intronic(hgvs_object):
    """
    Return whether the end position is intronic.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.end.offset != 0


def both_positions_are_intronic(hgvs_object):
    """
    Return whether both the start and end positions are intronic.
    """
    return (
        start_position_is_intronic(hgvs_object)
        and end_position_is_intronic(hgvs_object)
    )


def either_position_is_intronic(hgvs_object):
    """
    Return whether either the start or end position is intronic.
    """
    return (
        start_position_is_intronic(hgvs_object)
        or end_position_is_intronic(hgvs_object)
    )


def start_offset_is_positive(hgvs_object):
    """
    Return whether the start position has a positive intronic offset.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.start.offset > 0


def end_offset_is_positive(hgvs_object):
    """
    Return whether the end position has a positive intronic offset.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.end.offset > 0


def both_offsets_are_positive(hgvs_object):
    """
    Return whether both start and end offsets are positive.
    """
    return (
        start_offset_is_positive(hgvs_object)
        and end_offset_is_positive(hgvs_object)
    )


def either_offset_is_positive(hgvs_object):
    """
    Return whether either the start or end offset is positive.
    """
    return (
        start_offset_is_positive(hgvs_object)
        or end_offset_is_positive(hgvs_object)
    )


def start_offset_is_negative(hgvs_object):
    """
    Return whether the start position has a negative intronic offset.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.start.offset < 0


def end_offset_is_negative(hgvs_object):
    """
    Return whether the end position has a negative intronic offset.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.end.offset < 0


def both_offsets_are_negative(hgvs_object):
    """
    Return whether both start and end offsets are negative.
    """
    return (
        start_offset_is_negative(hgvs_object)
        and end_offset_is_negative(hgvs_object)
    )


def either_offset_is_negative(hgvs_object):
    """
    Return whether either the start or end offset is negative.
    """
    return (
        start_offset_is_negative(hgvs_object)
        or end_offset_is_negative(hgvs_object)
    )


def start_is_5_prime_utr(hgvs_object):
    """
    Return whether the start position is in the 5-prime UTR.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.start.base < 0


def end_is_5_prime_utr(hgvs_object):
    """
    Return whether the end position is in the 5-prime UTR.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.end.base < 0


def both_positions_are_5_prime_utr(hgvs_object):
    """
    Return whether both positions are in the 5-prime UTR.
    """
    return (
        start_is_5_prime_utr(hgvs_object)
        and end_is_5_prime_utr(hgvs_object)
    )


def either_position_is_5_prime_utr(hgvs_object):
    """
    Return whether either position is in the 5-prime UTR.
    """
    return (
        start_is_5_prime_utr(hgvs_object)
        or end_is_5_prime_utr(hgvs_object)
    )


def start_is_3_prime_utr(hgvs_object):
    """
    Return whether the start position is in the 3-prime UTR.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.start.datum == Datum.CDS_END


def end_is_3_prime_utr(hgvs_object):
    """
    Return whether the end position is in the 3-prime UTR.
    """
    is_object(hgvs_object)
    return hgvs_object.posedit.pos.end.datum == Datum.CDS_END


def both_positions_are_3_prime_utr(hgvs_object):
    """
    Return whether both positions are in the 3-prime UTR.
    """
    return (
        start_is_3_prime_utr(hgvs_object)
        and end_is_3_prime_utr(hgvs_object)
    )


def either_position_is_3_prime_utr(hgvs_object):
    """
    Return whether either position is in the 3-prime UTR.
    """
    return (
        start_is_3_prime_utr(hgvs_object)
        or end_is_3_prime_utr(hgvs_object)
    )

plug it in and see if we can replace all instances with them. If we need others, will add. Start small, work out

[John-F-Wagstaff]

It looks OK but you are still using longer names than needed, since these will all almost only be used in if statements we want shorter if we can avoid harming the self-documenting nature of the names. For example is could be removed from most of them, 3p or 5p probably works as well as 3_prime.

Also the both for the UTR is overkill, unless the start position ends up after the end position (which causes validation errors) both ends will always be inside the UTR if the innermost end of the span is in the UTR.

[John-F-Wagstaff]

so for 5' if the end is inside the 5' UTR the start will always be inside too, and for 3' UTR if the start is inside then the end will always be too.

[John-F-Wagstaff]

For brevity in the call sites, so we don't get excess multi line if statements, you may actually want to pull in the individual functions instead, that halves the line length for simpler calls.

[Peter-J-Freeman]

I think what I will do for the tewaking is get it all plugged in, remove what se dont use and refactor. The descriptive names help me think, but can shoten them as a lasr commit, as well as doing the specific imports, Otherwise brain may explode :)

[John-F-Wagstaff]

OK, I would find it harder that way round, but that depends on thinking styles, the output is the important concern.

[John-F-Wagstaff]

Is inv normally without a functioning ref here? (I know that it lacks a defined alt deliberately)
fetch_seq
is, annoyingly, one of the slowest functions that we use, if you can use ref for the del seq and reverse compliment on the ref instead for the ins seq that will be quicker in most if not all cases. You repeat this bellow too.

[John-F-Wagstaff]

BioPython Seq objects are quite heavy to make, and we do have internal reverse compliment functions already defined. Would that work better here.

[Peter-J-Freeman]

yes we do, a light revcomp function in utils. Done this now

[John-F-Wagstaff]

This needs considering as above with the using ref, vs extra seqrepo seq fetch comment.

[Peter-J-Freeman]

@John-F-Wagstaff. That's pretty much a wrap, other than a bit of clean up r.e. the module imports.

Outcomes key points

• Faster
• More accurate round-trip handling in some of the gap code
• NW_(NM) now treated as an independent entity, and will not produce primary assembly output if they don't match. And will fail if the intronic position is not correct wrt the supplied NW_(NM) boundary. I know this was something you wanted to fix.
• A configurable cache for the key validator db lookups
• Several bugs squished.
• And a necessary structural change r.e. VF for maintainability. VV also no longer used VF at all. Just VF using VV and also it now uses more VV functions than before, so can accept more input types

So, in terms of to do, its check it, see if it's all OK, clean up if required.

It looks like fetching from VVTA now has caches disabled. Is this correct. Do we want to look at this and add it as a configurable item in the same way for key lookups?

There are a few remaining evm creations left, but that will need to work on your clean up globals.

Hope its mostly OK

[John-F-Wagstaff]

I really really don't like integrating VariantFormatter into VariantValidator I did not outright object but this is not my prefered solution at all. EDIT even if you do want to do it it is a big change and should be done separately, not be folded into an already large pull request as a surprise bolt on to the last patch.

The VV code base is already too large and complex, this is not a good thing. Mixing the two code bases into the same project makes this worse. I had an in progress patch to remove the 1 VV usage of VF code and switch the VF tests to optional. (with the patch to remove of the VF CLI which you added to VV while this was in progress and put it into into VF currently to-do)

this feels like a step back to me.

At the very least given the nature of the code we do't ever want to add VF usage to VV, in future we want to make sure that VF is never called from VV.

[Peter-J-Freeman]

yep, we can discuss this in detail. There are a couple of reasons I prefer this setup personally, and can think about what to do going forward, but lets discuss all options. I'm open to all input. For now though, this really helped and it does make VF easier to maintain.

At the very least given the nature of the code we do't ever want to add VF usage to VV, in future we want to make sure that VF is never called from VV.

Done!

[Peter-J-Freeman]

If we separate it out again, we need to split the tests too. Thats for sure :)

So lets discuss this befoe the meeting with Nat and co

[John-F-Wagstaff]

Ideally if we do add it we would merge the history of the two projects as well, not just add the files but that is quite a complex undertaking, and would change the commit hashes on all our existing commits (post VF creation) in order to get them in the right order too.

[Peter-J-Freeman]

need to figure out why CI is failing too. All tests pass locally

[Peter-J-Freeman]

Just putting this here as a potential plan so I don't forget. Got to move on to other things tomorrow but will email you about a meeting

Proposed VariantValidator / VariantFormatter Packaging Model

We will retain VariantValidator and VariantFormatter in a single GitHub repository, but package them as two independently installable Python distributions.

This is a common and well-established software-development pattern, generally referred to as a monorepo. It is particularly suitable where closely related packages are developed, tested, and released together, but users may not need every component.

Repository Structure

The repository would be organised approximately as:

variantValidator/
├── VariantValidator/
├── VariantFormatter/
├── packaging/
│   ├── variantvalidator/
│   │   └── pyproject.toml
│   └── variantformatter/
│       └── pyproject.toml
├── pyproject.toml
├── tests/
├── docs/
└── ...

Each packaging pyproject.toml defines a separate Python distribution:

• VariantValidator packages and installs the VariantValidator code.
• VariantFormatter packages and installs the VariantFormatter code and declares VariantValidator as a dependency.

This gives users clean installation choices.

To install VariantValidator alone:

pip install VariantValidator

To install VariantFormatter:

pip install VariantFormatter

Installing VariantFormatter will also install its required VariantValidator dependency.

Development and pip install -e .

The repository root can retain a development-oriented pyproject.toml so that the existing development workflow:

git clone <repository>
cd variantValidator
pip install -e .

continues to install both VariantValidator and VariantFormatter in editable mode for developers working on the complete repository.

The package-specific configurations provide more selective development installs.

For VariantValidator only:

pip install -e ./packaging/variantvalidator

For VariantFormatter and its VariantValidator dependency:

pip install -e ./packaging/variantformatter

The intended behaviour is therefore:

pip install -e .
    → complete VV + VF development environment

pip install -e ./packaging/variantvalidator
    → VV only

pip install -e ./packaging/variantformatter
    → VF + required VV dependency

This means the normal developer workflow does not become more complicated simply because the released distributions have been separated.

Shared Versioning and Releases

Both packages can use setuptools_scm to derive their version from the same Git repository and Git tag.

For example:

Git tag: v4.1.0
       │
       ├── VariantValidator 4.1.0
       └── VariantFormatter 4.1.0

This avoids maintaining separate version numbers or synchronising releases between two repositories. A single release/tag identifies the corresponding versions of both tools.

PyPI Distribution

Although the source is maintained in one repository, the release process produces two independent distributions:

VariantValidator repository
        │
        ├── build VariantValidator
        │      └── publish → PyPI: VariantValidator
        │
        └── build VariantFormatter
               └── publish → PyPI: VariantFormatter

These can initially be built and uploaded manually using build and twine, and subsequently automated through GitHub Actions and PyPI Trusted Publishing.

For example, the distributions could be built with:

python -m build packaging/variantvalidator
python -m build packaging/variantformatter

and checked before publication using:

twine check dist/*

The packages can then be uploaded to PyPI using twine, or publication can be handled automatically by GitHub Actions.

Resulting Model

The resulting model provides the advantages of both approaches:

• one GitHub repository;
• one development workflow;
• one shared version and release history;
• one integrated test suite;
• two independently installable Python distributions;
• users of VariantValidator do not have to install VariantFormatter;
• installing VariantFormatter automatically installs its VariantValidator dependency; and
• developers working on the complete project retain the simple pip install -e . workflow used today.

This also preserves the natural dependency relationship between the two tools:

VariantFormatter
       │
       │ depends on
       ▼
VariantValidator

VariantFormatter depends on VariantValidator, but VariantValidator does not require VariantFormatter.

The monorepo therefore keeps the two closely related tools together for development, testing, versioning, and release management while providing clean, conventional Python packaging for end users.

For me, it makes it a lot easier to maintain. Especially when developing VF.

[John-F-Wagstaff]

Having had a more detailed look through the first few using git on my own machine, rather than the github interface, it is looking fine so far, modulo a few niggles that only might be worth fixing, but it also looks like it would be relatively easy to split up some of the early patches into per-file/ function sections.

Would you like me to have a look with an eye to possibly re-basing them into something more focused? Depending on how things go in later patches, and what you think of the suggestions, there are also a few places that I could pop in fixes directly to the patch, instead of appending them on the end too if I do.

[Peter-J-Freeman]

Hi John,

Yes, if its OK. Any niggles you spot it is worth hitting them now. As you saw, I spotted a few too in the last few merges. Let's take this oportunity to have a good thorough clean up and optimisation. Plus, anything we can do to sort the copyright too with easy wins.