Issue #356: Show first and last events instead of only first ones when displaying an EventSet #362
Conversation
…n displaying an EventSet
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Hey @jtaylor205! Great work so far :) A couple of comments:
- The elipsis row should be shown in the middle of the first 3 and last 3 rows, not at the end. See how pandas does it:
- You'll need to accept the Google CLA for the contribution to be merged, see the comment in this PR from the google-cla bot for instructions.
- There's tests checking the eventset is displayed as expected that you'll need to modify: see the
test_html_repr_no_limitsandtest_html_repr_limitsintemporian/implementation/numpy/data/test/event_set_test.py - Please post a screenshot of the displayed eventset in the PRs description to see what it looks like.
Thanks in advance!
There was a problem hiding this comment.
Screenshots looking perfect!
My comments above about removing [:num_timestamps] and not reconstructing the list still hold.
Same for the tests: you can run them with bazel test --config=macos //temporian/implementation/numpy/data/test:event_set_test --test_output=errors. You'll see that both test_html_repr_no_limits and test_html_repr_limits fail, because the display changed - you need to update the html in the files test_html_repr_no_limits_golden and test_html_repr_limits_golden - you can do this manually, or by copying the output of your new code.
|
Formatting check is failing - ensure you have black is correctly configured (should be running on save by default if no user VSCode setting is overriding the workspace's ones) |
|
Please merge main branch into your PR's branch so that tests are ran on it (fixed in #363) |
There was a problem hiding this comment.
LGTM after my last comment is addressed. Great work @jtaylor205 and congrats on your first open source contribution! 🎉
Will wait for a review from @javiber before merging.
Edit: found some breaking behavior that needs to be addressed.
| if num_timestamps > max_timestamps: | ||
| # Timestamp + features + <... column if was added> | ||
| row = [ELLIPSIS] * (1 + len(visible_feats) + int(has_hidden_feats)) | ||
| # Create ellipsis row between first half and last half if more than man_timestamps entries |
There was a problem hiding this comment.
correct typo, max_timestamps
There was a problem hiding this comment.
Testing the display manually it seems to break when setting tp.config.display_max_events = None (which should have the same behavior as setting it to 0). Please write a test that does the same thing as test_html_repr_no_limits but sets all variables to None instead of 0, and checks the result against the same golden file, and fix the code to make it work.
Thanks!
|
Hi @jtaylor205 thanks for the contribution! Testing this locally I found that setting 
|
|
Hi @javiber , I have fixed that problem. Below is what it now shows. I will upload my changes now |
|
Congrats on your first open-source contribution @jtaylor205! Good job 💪🏼 |
Adjusted displays_util.py to display 7 at a time until more than 7 timestamps are created, in which the ellipses row is implemented after the first three rows, with the last 3 rows following that row.