Skip to content

Conversation

@jthorton
Copy link
Collaborator

@jthorton jthorton commented Sep 22, 2025

Fixes #1493 by changing the default atom mapper used in the CLI to Kartograf.

Note:

  • I added the flag to map hydrogens to hydrogens only, as these are later removed in the protocol anyway, it should make the mapping visualisation clearer as well, and this is planned to be the new default setting in kartograf in the future.

Checklist

  • Added a news entry

Developers certificate of origin

@jthorton jthorton marked this pull request as draft September 22, 2025 16:32
@codecov
Copy link

codecov bot commented Sep 22, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.20%. Comparing base (2d3af6a) to head (aeb26c8).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
- Coverage   95.25%   93.20%   -2.05%     
==========================================
  Files         172      172              
  Lines       14482    14487       +5     
==========================================
- Hits        13795    13503     -292     
- Misses        687      984     +297     
Flag Coverage Δ
fast-tests 93.20% <100.00%> (?)
slow-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

lgtm on my end, although I'm concerned that no tests failed when you did that 🤔 is there a long test that checks the network generation maybe?

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Please let @atravitz be the approver here

@jthorton
Copy link
Collaborator Author

We do have a planning test for the CLI, but in this case we are lucky and got the same edges (tyk2 has good alignment and we made the edge order reproducible!), so no test failed. I have added a check for the default settings used in the CLI by matching some info in the output stream. This could be improved in future when we have provenance on the LigandNetwork and AlchemicalNetwork objects.

@jthorton jthorton requested a review from atravitz September 25, 2025 08:55
@jthorton jthorton marked this pull request as ready for review September 25, 2025 08:55
@mikemhenry
Copy link
Contributor

wow called out
image
did we decide that this isn't a 2.0 breaking change since we don't promise that the CLI defaults don't change?

@IAlibay
Copy link
Member

IAlibay commented Oct 1, 2025

did we decide that this isn't a 2.0 breaking change since we don't promise that the CLI defaults don't change?

Yeah - board approved it too. We're selling it as "this is what was used in the industry benchmarks".

Copy link
Contributor

@atravitz atravitz left a comment

Choose a reason for hiding this comment

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

LGTM, and I agree with @jthorton about adding more tests once we store this info.

Co-authored-by: Alyssa Travitz <31974495+atravitz@users.noreply.github.com>
@jthorton jthorton enabled auto-merge October 7, 2025 13:36
@github-actions
Copy link

github-actions bot commented Oct 8, 2025

No API break detected ✅

@jthorton jthorton merged commit 0cbfee2 into main Oct 8, 2025
13 checks passed
@jthorton jthorton deleted the default_atom_mapper branch October 8, 2025 09:21
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.

Change default atom mapper to Kartograf!

5 participants