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

[SofaPython] print cleaning + SofaPython quaternion dot product #404

Merged
merged 11 commits into from
Oct 18, 2017

Conversation

ErwanDouaille
Copy link
Contributor

@ErwanDouaille ErwanDouaille commented Sep 15, 2017

This pull request contains few cleaning cout/print/comments.

@ChristianDuriez also added the dot poduct for SofaPython Quaternion


This PR:

  • builds with SUCCESS for all platforms on the CI.
  • does not generate new warnings.
  • does not generate new unit test failures.
  • does not generate new scene test failures.
  • does not break API compatibility.
  • is more than 1 week old (or has fast-merge label).

Reviewers will merge only if all these checks are true.

@sofabot
Copy link
Collaborator

sofabot commented Sep 15, 2017

Thank you for your pull request!
Someone will soon check it and start the builds.

@hugtalbot hugtalbot added pr: status to review To notify reviewers to review this pull-request location: plugins labels Sep 15, 2017
@guparan guparan changed the title [cleaning][SofaPython] print cleaning + SofaPython quaternion dot product [SofaPython] print cleaning + SofaPython quaternion dot product Sep 18, 2017
@guparan guparan added the pr: clean Cleaning the code label Sep 18, 2017
@guparan
Copy link
Contributor

guparan commented Sep 18, 2017

[ci-build]

@guparan guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Sep 19, 2017
@damienmarchal
Copy link
Contributor

[ci-build]

I add a comment to explain the usage of the product
@damienmarchal damienmarchal added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Sep 22, 2017
@hugtalbot
Copy link
Contributor

[ci-build]

1 similar comment
@hugtalbot
Copy link
Contributor

[ci-build]

@guparan guparan added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Oct 11, 2017
@@ -35,6 +35,10 @@ def angle(q):
"""get angle in rad"""
return 2.0* math.acos(re(q))

def product(qa, qb):
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
I think your "product" function does exactly the same as the "prod" function just below (I checked).
I prefer yours, because of the comment and the fact that the formula is easier to understand/verify.
So maybe we should remove the "prod" function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi,
Python noob here, I see hstack and numpy, I think performance issue. @matthieu-nesme could you help?

Copy link
Member

Choose a reason for hiding this comment

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

If it is the same, why adding a new function?
Rather than removing the prod function (that is used in existing code), I would keep it, but replace its code with the new implem (personnaly I would keep the old implementation as a comment as I find it clearer/closer to the math).

But please be sure that is exactly the same.

Also, rather than using directly the [] operator, I would use re(a), im(b)[0], im(b)[1], im(b)[2]. That way it is easier to read, and independent to the scalar order.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a performance issue in both cases:

  • hstack + list (!) is useless and can be replaced by res = np.zeros(4); res[:3] = ...; res[-1] = ...;
  • yet the proposed version is probably even less efficient since it does all the computations in python which is waaay slower than letting numpy doing its stuff in C

@ChristianDuriez
Copy link
Contributor

ChristianDuriez commented Oct 13, 2017

Ok... Do we all consider that numpy is mandatory when using SOFA with python ?

It is just a question so that people are aware of that dependency. But I agree to remove the new implementation if less performant .

Erwan will do it

@guparan
Copy link
Contributor

guparan commented Oct 16, 2017

Yes numpy is a mandatory part of SofaPython which is (almost) a mandatory part of SOFA itself.

@ErwanDouaille ErwanDouaille added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Oct 16, 2017
@damienmarchal
Copy link
Contributor

[ci-build]

@guparan guparan added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels Oct 18, 2017
@hugtalbot hugtalbot merged commit f4df217 into sofa-framework:master Oct 18, 2017
@ErwanDouaille ErwanDouaille deleted the Defrost_cleaning_week37 branch October 18, 2017 12:33
@guparan guparan added this to the v17.12 milestone Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: clean Cleaning the code pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants