adapting the Konnektor behavior to openfe.

[RiesBen]

if no scorer is provided to the max network planners, the std. behavior is to take the first possible mapping and be done. It used to use the last possible mapping.

this change is a minor adaptation to the old openFE behavior.fixes unit tests.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.76%. Comparing base (8c62d86) to head (07c216d).

Files with missing lines	Patch %	Lines
.../generators/heuristic_maximal_network_generator.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
- Coverage   86.81%   86.76%   -0.05%     
==========================================
  Files          49       49              
  Lines        1577     1579       +2     
==========================================
+ Hits         1369     1370       +1     
- Misses        208      209       +1     

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

[RiesBen]

note Mac CI is failing currently everywhere in this Konnektor Repo, and we assume it is an rdkit version issue, if I understood it correctly, right @mikemhenry ?

[IAlibay]

@RiesBen this PR seems to be missing tests, is this intentional?

[RiesBen]

@RiesBen this PR seems to be missing tests, is this intentional?

hmmmm... the thing is, the effect of the change was not tested before.
So ideally one would add tests now. On the other hand this tries mainly to make the openFE-repo happy as the behavior is tested there.

I think it would be more beautiful to also test the behaviour here and I will try to add them if I have time.

[atravitz]

added as an issue here so this PR is unblocked.

[atravitz]

I am more concerned that this is considered covered by tests, but the tests did not break when this change was made.

[atravitz]

To clarify:

The prior behavior was to use the first mapping of the last mapper passed to mappers.

The current behavior takes the first mapping of the first mapper passed to mappers.

In the case of multiple mappers, but no scorer, I think we should raise a warning. It might be confusing if a user is passing in multiple mappers and thinks both are being used, but it's quietly defaulting to the first mapper.