-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
466 add delay in testing and planned migration #866
466 add delay in testing and planned migration #866
Conversation
Please remember to account for the benchmark, as testing is really computationally extensive :-D |
…Trips and Migration rules
…Trips and Migration rules
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
==========================================
+ Coverage 96.14% 96.42% +0.28%
==========================================
Files 131 132 +1
Lines 11049 11061 +12
==========================================
+ Hits 10623 10666 +43
+ Misses 426 395 -31 ☔ View full report in Codecov by Sentry. |
Basic functionality is implemented and is ready for review. However, the benchmark test is much slower than the |
Improved performance for the implementation with test results stored in the past. Branch main:Benchmark Time CPU Iterationsabm_benchmark/abm_benchmark_50k 2281 ms 2255 ms 1 Branch #866:Benchmark Time CPU Iterationsabm_benchmark/abm_benchmark_50k 2391 ms 2367 ms 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes. I have a couple of suggestions for small improvements.
Is the benchmark up to date or has it improved with the new way of testing in the past?
Apparently, there have been a lot of outdated comments that I made earlier. Please check if they are still valid and just resolve them if they are outdated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my side this looks good. I just did a high-level review.
Questions i had have already been questioned by david and were answered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! There are only two minor things that I found now which could be slightly improved.
Changes and Information
Please briefly list the changes made, additional Information and what the Reviewer should look out for:
Merge Request - Guideline Checklist
Please check our git workflow. Use the draft feature if the Pull Request is not yet ready to review.
Checks by code author
Checks by code reviewer(s)
closes #466