Fix slow displacement test #6652
Conversation
greenape
added
tests
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to bc51970 in 43 seconds
More details
- Looked at
41lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_rh3vqPM03BTJULMG
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6652 +/- ##
=======================================
Coverage 92.30% 92.30%
=======================================
Files 268 268
Lines 10590 10590
Branches 856 856
=======================================
Hits 9775 9775
Misses 676 676
Partials 139 139 ☔ View full report in Codecov by Sentry. |
greenape
force-pushed
the
fix-slow-displacement-test
branch
from
bc51970
to
f7c5ac8
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f7c5ac8 in 1 minute and 19 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_WquEHFAKETXPVxhJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Passing run #22840 ↗︎
Details:
Review all test suite changes for PR #6652 ↗︎ |
|||||||||||||||
greenape
force-pushed
the
fix-slow-displacement-test
branch
from
f7c5ac8
to
3c04dcc
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3c04dcc in 1 minute and 22 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_MXWGV53071CzUBPA
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| assert isnan(df.loc[subscriber].value) | ||
| sub = df.loc[subscriber].value | ||
| assert sub is None |
There was a problem hiding this comment.
The test expects None for a subscriber with no activity, which contradicts the typical use of NaN for missing numerical data in pandas. Consider reverting this to check for NaN to align with standard practices and the test's intent.
| assert sub is None | |
| assert isnan(sub) |
greenape
force-pushed
the
fix-slow-displacement-test
branch
from
3c04dcc
to
0df9736
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 0df9736 in 2 minutes and 6 seconds
More details
- Looked at
32lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_shrg1ym9XtEwUD6t
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
|
||
| assert isnan(df.loc[subscriber].value) | ||
| sub = df.loc[subscriber].value | ||
| assert sub is None |
There was a problem hiding this comment.
The test name and assertion contradict each other. The test is named test_subscriber_with_home_loc_but_no_calls_is_nan but asserts that the value is None instead of NaN. If the behavior has changed intentionally, consider renaming the test to reflect the new behavior or adjusting the assertion to match the expected behavior.
| assert sub is None | |
| assert isnan(sub) |
I have:
Description
Makes a couple of changes to the tests for the
Displacementclass, which speed up the tests substantially. Namely, storing the reused reference location, and supplying a subscriber subset.Summary:
Optimized
Displacementclass tests by caching reference locations and limiting data scope, with modifications to specific tests and methods.Key points:
Displacementclass tests by caching reference locations and limiting data scope.test_get_all_users_in_reference_locationandtest_subscriber_with_home_loc_but_no_calls_is_naninflowmachine/tests/test_displacement.py.store()method to cachedaily_locationresults.subscriber_subsetto limit data scope in tests.Generated with ❤️ by ellipsis.dev