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

352: Enhancement: Support tp.to_numpy() for temporian #378

Merged
merged 10 commits into from Mar 7, 2024

Conversation

nagavenkateshgavini
Copy link
Contributor

@nagavenkateshgavini nagavenkateshgavini commented Feb 23, 2024

PR to #352

  • add support for tp.to_numpy()
  • Provide timestamps: bool = True argument
  • Provide timestamp_to_datetime: bool = True argument
  • Add tests in temporian/io/test/numpy_test.py
  • test_correct
  • test_no_index
  • test_no_timestamps
  • test_timestamp_to_datetime_param
  • test_empty_event_set
  • Add doc string

Copy link
Collaborator

@javiber javiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Code looks good!

I left some ideas to improve the tests

temporian/io/numpy.py Outdated Show resolved Hide resolved
temporian/io/test/numpy_test.py Outdated Show resolved Hide resolved
temporian/io/test/numpy_test.py Outdated Show resolved Hide resolved
temporian/io/test/numpy_test.py Show resolved Hide resolved
@nagavenkateshgavini
Copy link
Contributor Author

@javiber
I've addressed all your feedback, with the only remaining question being whether to sort the array before comparing it or not.

Copy link
Collaborator

@javiber javiber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will tackle the sorting in another issue, I left you a small comment but there is something more important that I missed in the first review, we need to format the test file using black.

  1. run black temporian/io/test/numpy_test.py. Black should already be installed, you might need to activate the virtualenv (poetry shell) or pre-pend poetry run to the command
  2. add, commit and push the changes to the file

temporian/io/test/numpy_test.py Outdated Show resolved Hide resolved
@javiber
Copy link
Collaborator

javiber commented Mar 1, 2024

@nagavenkateshgavini There is still a formatting error, you can run black --check . locally to get more info about the problem

@nagavenkateshgavini
Copy link
Contributor Author

nagavenkateshgavini commented Mar 1, 2024

@javiber
just made a commit after reformatting

$ black --check .                        
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install black[jupyter]``
All done! ✨ 🍰 ✨
307 files would be left unchanged.

@nagavenkateshgavini
Copy link
Contributor Author

@javiber
Could you please provide your approval for the pending workflow checks?

@ianspektor
Copy link
Collaborator

Hey @nagavenkateshgavini! Docs build was fixed in #386, please merge main to your PR to fix the jupyter: command not found error. Approving the workflows now. Thanks!

@javiber javiber changed the title Add tp.to_numpy() feature 352: Add tp.to_numpy() feature Mar 4, 2024
temporian/io/numpy.py Outdated Show resolved Hide resolved
@nagavenkateshgavini nagavenkateshgavini changed the title 352: Add tp.to_numpy() feature 352: Enhancement: Support tp.to_numpy() for temporian Mar 4, 2024
@nagavenkateshgavini
Copy link
Contributor Author

@ianspektor
I merged my branch with the main, and the docs test was successful.
Could you please approve the workflows? Hopefully, this will be the last workflow test for this PR.

cc: @javiber

@javiber javiber merged commit 6e8cd65 into google:main Mar 7, 2024
14 checks passed
@ianspektor
Copy link
Collaborator

Congrats and thank you for your contribution @nagavenkateshgavini! 🥳

@nagavenkateshgavini nagavenkateshgavini deleted the feature_#352 branch March 7, 2024 14:46
@nagavenkateshgavini
Copy link
Contributor Author

Thank you, @ianspektor, for the amazing insights; this is truly fascinating. And a huge thank you to @javiber for your constant support and guidance throughout this process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants