Skip to content
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

Show first and last events instead of only first ones when displaying an EventSet #356

Closed
ianspektor opened this issue Jan 22, 2024 · 11 comments · Fixed by #362
Closed

Show first and last events instead of only first ones when displaying an EventSet #356

ianspektor opened this issue Jan 22, 2024 · 11 comments · Fixed by #362
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@ianspektor
Copy link
Collaborator

ianspektor commented Jan 22, 2024

Change the HTML display of an EventSet to display the first 3 and last 3 events on each index value, instead of showing the first 5. See how pandas displays DataFrames display for reference.

The main display function is display_html in temporian/implementation/numpy/data/display_utils.py.

@ianspektor ianspektor added enhancement New feature or request good first issue Good for newcomers labels Jan 22, 2024
@jtaylor205
Copy link
Contributor

I can take this one if available

@ianspektor
Copy link
Collaborator Author

Hey @jtaylor205 - all yours!

Let me know if any doubts arise or there's any design decisions you'd rather run by us before going ahead with the implementation.

Thanks!

@jtaylor205
Copy link
Contributor

Thanks Ian! I will get to work on it within the week. I have to complete a "good first issue" task for a class I am taking and ned to get this approved so I can get to it.

@jtaylor205
Copy link
Contributor

Hi Ian,
quick question on how to install my edited version to view changes. I am trying to run pip install -e . from the directory I have my forked repository in, but I keep getting the error ERROR: Could not build wheels for temporian, which is required to install pyproject.toml-based projects . Is there something I am doing wrong there or another way to run my modified version? I'm doing this through terminal on my M2 MacBook Pro

@ianspektor
Copy link
Collaborator Author

Could you provide more detailed logs of the error? pip install -e . works for me, though it's not what I use to develop and might just be because poetry build has already generated necessary intermediate assets.

Safer option is to try to build the wheel with poetry with poetry build, then install the wheel that is generated under dist/.

Note that you need to have bazel installed, see Environment setup.

@jtaylor205
Copy link
Contributor

jtaylor205 commented Jan 30, 2024

Thanks so much for that help. It makes a lot more sense and it worked. I have got it into jupyter and will work on getting the task done!

@ianspektor
Copy link
Collaborator Author

No worries! Hope my college had "forced" me to contribute to OSS too :)

An EventSet with 8 events and no index should be enough for the very basic test (since up to 7 events should be shown, but with 8 we should already show the first 3, then a "..." row, and then the last 3). Note however that this number should be configurable through the print_max_events variable in temporian/utils/config.py as it is now.

You can generate an EventSet with a single feature, no index, and N events like this for example:

N = 8
values = list(range(N))
evset = tp.event_set(
    timestamps=values,
    features={"f": values},
)

@ianspektor
Copy link
Collaborator Author

Answered to the original non-edited comment 😉. Eager to review your work!

@ianspektor
Copy link
Collaborator Author

Also, feel free to join our Discord if you'd like quick help while implementing this.

@jtaylor205
Copy link
Contributor

Yeah I had edited it because I had found out how to make the values. In regards to what you just said though, why would up to 7 events be shown? If you only are looking for the top 3 and bottom 3, would you not want the top three, a "..." row, and then the bottom 3? As for the discord, if I really run into trouble, I will look for help there, but I hope to be able to work it out on my own.

jtaylor205 added a commit to jtaylor205/temporian that referenced this issue Jan 31, 2024
@ianspektor
Copy link
Collaborator Author

Showing first 3, then ..., then last 3 takes up 7 rows, so if the EventSet has 7 events I'd propose showing all 7 (since it takes up the same space but shows an extra row), and only start showing the ... for 8 and above :)

javiber pushed a commit that referenced this issue Feb 26, 2024
…n displaying an EventSet (#362)

* Issue #356: Show first and last events instead of only first ones when displaying an EventSet

* Adjusted to show all 7 items

* Set default display_max_events to 7

* Adjusted calculation on split table based on display_max_events

* Edited Based on Ian's comments

* Removed '7' from being hardcoded in comment

* Updated html files to reflect changes made

* Made changes based off of comments

* formatted file with black

* Fixed golden html files

* Fixed max = 1 error

* Fixed 'NONE' error and typo

* Made new test with 'NONE'

* Formatted event_set_test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
2 participants