Skip to content

Conversation

@anupam-banerjee
Copy link
Contributor

Fix _getBiomoltrans to robustly parse mmCIF oper_expression: accept alphanumeric op IDs (X0, P), expand ranges anywhere (incl. inside lists), and handle products of groups like (X0)(1-5).

Copy link
Contributor

@jamesmkrieger jamesmkrieger left a comment

Choose a reason for hiding this comment

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

looks good and I've tested pieces and it seems to work as expected.

It would be good to have a unit test for this too

Copy link
Member

@AnthonyBogetti AnthonyBogetti left a comment

Choose a reason for hiding this comment

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

Let's come up with a unit test for this before merging.

@AnthonyBogetti
Copy link
Member

test_ciffile.py
Here is a modified test_ciffile.py to cover the changes in this PR. @anupam-banerjee, please test on your code and if it works, let's merge this.

@jamesmkrieger
Copy link
Contributor

test_ciffile.py Here is a modified test_ciffile.py to cover the changes in this PR. @anupam-banerjee, please test on your code and if it works, let's merge this.

Why do we need a modified version? Surely it’s better to have an additional one and keep checking that old ones work too?

@AnthonyBogetti
Copy link
Member

AnthonyBogetti commented Dec 3, 2025

test_ciffile.py Here is a modified test_ciffile.py to cover the changes in this PR. @anupam-banerjee, please test on your code and if it works, let's merge this.

Why do we need a modified version? Surely it’s better to have an additional one and keep checking that old ones work too?

The test_ciffile.py file itself is modified, but all existing tests should have been left untouched. The new test to cover the changes in this PR was added to the end of the file. Sorry, I should have been more clear.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants