Fix ring hybridization filter

[jthorton]

Fixes #62.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (ca54878) to head (f670838).
Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #65   +/-   ##
=======================================
  Coverage   96.60%   96.60%           
=======================================
  Files          13       13           
  Lines         618      618           
=======================================
  Hits          597      597           
  Misses         21       21           

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

[IAlibay]

Thanks lgtm!

We probably should add rever and add a news item at some point.

[hannahbaumann]

Thanks, @jthorton , this looks good to me!
I just have one general question to this filter:
Right now, this only removes / disallows mappings between an aromatic and aliphatic ring. Would we also want to disallow mapping e.g. two aliphatic rings with different hybridization of some atoms (e.g. cyclohexane and cyclohexene) or should those cases be handled in the atom hybridization change filter?

[jthorton]

@hannahbaumann great point, I think the atomwise hybridization filter should handle those cases currently but it would be good to extend this filter also to do the same. I believe that using the atom hybridization and map_exact_ring_matches_only=True should do the same filtering as this filter on rings but handle non-ring cases as well so for users that only want to handle ring cases, we should make the behaviour consistent. I'll open another issue for that and look to do it in another PR though to keep this one simple.

[hannahbaumann]

Thanks @jthorton , that sounds great!