Eval metrics and circular import bug fix.

[Lilferrit]

Implemented bug fixes to resolve #378 and #379. Also implemented a unit test for ModelRunner.log_metrics to test for future incorrect behavior.

[codecov]

Codecov Report

Attention: Patch coverage is 96.03960% with 4 lines in your changes missing coverage. Please review.

Project coverage is 94.37%. Comparing base (aee3534) to head (60524af).
Report is 3 commits behind head on dev.

Files with missing lines	Patch %	Lines
casanovo/denovo/model_runner.py	93.02%	3 Missing ⚠️
casanovo/casanovo.py	96.29%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #380      +/-   ##
==========================================
+ Coverage   94.32%   94.37%   +0.05%     
==========================================
  Files          12       13       +1     
  Lines        1039     1102      +63     
==========================================
+ Hits          980     1040      +60     
- Misses         59       62       +3     

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

[bittremieux]

Looks good. One more thing to test/verify: multiple skipped spectra in a row. And then I think switching to None for skipped spectra instead of an empty string, and updating aa_match_batch to handle this, improves clarity.

[Lilferrit]

The updated aa_match_batch now handles amino acid precision for skipped spectra differently. For example, say for the ground truth peptides PEP and PET the model only (correctly) infers PEP and skips PET, the amino acid precision will now be 100%. Before, the first amino acid in any skipped spectrum would be treated as an incorrect inference, while the rest wouldn't be counted, so the peptide precision for the last case would be .75, and I'm not sure if that was correct.

[bittremieux]

Looks good. Let's finish the discussion on Slack how to account for skipped spectra when calculating amino acid precision before merging.

I realized a situation where the evaluation might fail though: If we have multiple predictions per spectrum (i.e. top_match in the config > 1). I think that this might have failed in the previous implementation as well though. And it's not super obvious how to handle this situation (or if we should—but maybe at least a check/warning).

[Lilferrit]

I realized a situation where the evaluation might fail though: If we have multiple predictions per spectrum (i.e. top_match in the config > 1). I think that this might have failed in the previous implementation as well though. And it's not super obvious how to handle this situation (or if we should—but maybe at least a check/warning).

To me the most intuitive way to handle this is to only evaluate the highest confidence PSM for each spectrum. If I'm understanding everything correctly the current implementation just matches whatever PSM happens to be first in writer.psms to the ground truth spectra, which might be ok (?), but as you said I'm also not sure if it's worth implementing special handling for the case where top_match > 1. For now I'll just add a warning that pops if top_match is not one.

[wsnoble]

To me the most intuitive way to handle this is to only evaluate the highest confidence PSM for each spectrum.

I agree with this.

[bittremieux]

Minor comments.