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.

Sign up for GitHub

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

Jump to bottom

Performance enhancements #107

Merged
merged 6 commits into from
Jul 16, 2019
Merged

Performance enhancements #107

merged 6 commits into from
Jul 16, 2019

Conversation

erikwijmans
Copy link
Contributor

Motivation and Context

Slightly faster implementations of things since a lot of the internal logic now uses magnum types instead of numpy types.

How Has This Been Tested

Via a profiling script using python's cProfile package.

Types of changes

Performance enhancement

@erikwijmans erikwijmans requested review from mosra and msavva July 15, 2019 20:34
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jul 15, 2019
mathfac
mathfac approved these changes Jul 16, 2019
View reviewed changes
Copy link
Contributor

@mathfac mathfac left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for profiling that. Checked logic of the changes and it looks good to me. Tests are passing with FPS: 1333.3, let's merge it.

@mathfac mathfac merged commit fa17d51 into master Jul 16, 2019
@mathfac mathfac deleted the fix-perf branch July 16, 2019 07:51
@mosra
Copy link
Collaborator

mosra commented Jul 16, 2019 •
edited
Loading

You merged so fast I didn't have time to review 😅

One minor API usability thing -- I'm not sure if the new property wouldn't better be named absolute_translation instead of translation, all other absolute properties are prefixed with absolute_ as well. It might confusing compared to e.g. transformation, which is relative to parent, not absolute. This was my concern in the original pre-Magnum bindings as well (which had the same function), but I removed all that code anyway.

Besides that, does this improve performance beyond what was there before #64 got merged? Or do we still have some catching-up to do?

@erikwijmans
Copy link
Contributor Author

Performance seems to be about where it was before #64, if not slightly better, with #104 + this PR

@erikwijmans erikwijmans mentioned this pull request Jul 16, 2019
Merged
Ram81 pushed a commit to Ram81/habitat-web-sim that referenced this pull request Dec 10, 2020
@erikwijmans @mathfac
Performance enhancements with using magnum instead of numpy (facebook…
821a2b2 
…research#107)

* Change to using buffer protocol instead of .numpy()
* Slightly faster implementations of things since a lot of the internal logic now uses magnum types instead of numpy types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathfac mathfac mathfac approved these changes

@mosra mosra Awaiting requested review from mosra

@msavva msavva Awaiting requested review from msavva

Assignees
No one assigned
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@erikwijmans @mosra @mathfac @facebook-github-bot