💚 Resolve irregularly failing tests

[ystade]

Description

The test of the NA-mapper failed from time to time. The cause seems to be the size of the entangling zone that is too narrow when the mapper comes up with a suboptimal run.

Checklist:

• The pull request only contains commits that are related to it.
• I have added appropriate tests and documentation.
• I have made sure that all CI jobs on GitHub pass.
• The pull request introduces no new warnings and follows the project's style guidelines.

[codecov]

Codecov Report

Attention: Patch coverage is 84.79532% with 26 lines in your changes missing coverage. Please review.

Project coverage is 90.4%. Comparing base (90e29dd) to head (860b087).
Report is 81 commits behind head on main.

Files with missing lines	Patch %	Lines
src/na/NAMapper.cpp	83.8%	25 Missing ⚠️
src/na/NAGraphAlgorithms.cpp	93.7%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #469     +/-   ##
=======================================
- Coverage   90.4%   90.4%   -0.1%     
=======================================
  Files         86      86             
  Lines       9965    9975     +10     
  Branches    1696    1696             
=======================================
+ Hits        9012    9018      +6     
- Misses       953     957      +4     

Flag	Coverage Δ
cpp	90.1% <84.7%> (-0.1%)	⬇️
python	95.9% <ø> (ø)

Files with missing lines	Coverage Δ
src/na/NAGraphAlgorithms.cpp	93.0% <93.7%> (+<0.1%)	⬆️
src/na/NAMapper.cpp	89.3% <83.8%> (-0.5%)	⬇️

[ystade]

@burgholzer I substituted all vector accesses that happened with the []-operator with a call to the method .at() to enable bound checks, see 0f40f70. Not sure whether we want to keep this because of performance considerations.

The issue of the failing test was actually not a Windows problem; on other machines, the error could have happened as well and would have resulted in a segmentation fault. I adapted the test case now accordingly.

[burgholzer]

Thanks for digging into this!
Overall looks good to me.
I just skimmed the changes and added suggestions where I deemed that the range checked access is definitely not necessary because a range check is already conducted before that.
You should be able to simply batch up all suggestions into a single commit on GitHub.
I'd keep the other at's just for safety.

[ystade]

Thanks for digging into this! Overall looks good to me. I just skimmed the changes and added suggestions where I deemed that the range checked access is definitely not necessary because a range check is already conducted before that. You should be able to simply batch up all suggestions into a single commit on GitHub. I'd keep the other at's just for safety.

Thanks a lot for going through them thoroughly. I will add these suggestions.

[ystade]

Do I need to care about the failing coverage test since the same branches are tested as before?

[burgholzer]

Do I need to care about the failing coverage test since the same branches are tested as before?

Good question. In principle, I'd say not really. I am ok with merging this as-is. If you feel bored, you could probably just write an additional test that covers some of the previously untouched lines and the CI would be totally happy.

[ystade]

I am working on proper tests right now that actually test the functionality (not only the termination). This I think makes more sense after all.

[burgholzer]

I am working on proper tests right now that actually test the functionality (not only the termination). This I think makes more sense after all.

Makes sense. I'd guess you'd prefer to do that in a separate PR, right? Or should I wait before merging this one?

[ystade]

I am working on proper tests right now that actually test the functionality (not only the termination). This I think makes more sense after all.

Makes sense. I'd guess you'd prefer to do that in a separate PR, right? Or should I wait before merging this one?

I'll open a new PR for that soon.