Skip to content

Conversation

@Akshat111111
Copy link

@Akshat111111 Akshat111111 commented Oct 25, 2023

PR summary

[MNT]: Reducing repetitive computations and simplified rotation calculation in proj3d.py #27186
reduced the repetitive computations by storing the repetitive values like (1/dx,1/dy,1/dz). Apart from it using the Numpy function effectively for vectored operations.
And the rotation_about_vector function is also enhanced by removing not required intermediate variables.
As well as implemented vectorized division and column stacking in multiple places.

PR checklist

Reducing repetitive computations and simplified rotation calculation
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@QuLogic
Copy link
Member

QuLogic commented Oct 25, 2023

There is no reason to change docstrings to comments, or remove deprecations from functions. It makes it very difficult to determine what it is you actually intend here as an optimization. Please remove the superfluous changes so that it is clearer what is going on here. Also, did you run any benchmarks to show that these are indeed faster?

@rcomer rcomer added the hacktoberfest: spam invalid for hacktoberfest label Oct 25, 2023
@rcomer
Copy link
Member

rcomer commented Oct 25, 2023

@Akshat111111 over the last couple of weeks you have opened PRs in a variety of projects. In several of these, the maintainers have questioned the purpose of the change. Some have taken the time to give a detailed review. So far, none have been merged. This does not look to me like a good use of maintainers' time.

If you want to make a genuine contribution, please have a look through projects' issues list to find out what changes they want.

@tacaswell
Copy link
Member

I am going to close this for now. If you want to clean this up by removing the extraneous changes please do so and ask to have the PR re-opened.

@tacaswell tacaswell closed this Oct 25, 2023
@Akshat111111
Copy link
Author

@Akshat111111 over the last couple of weeks you have opened PRs in a variety of projects. In several of these, the maintainers have questioned the purpose of the change. Some have taken the time to give a detailed review. So far, none have been merged. This does not look to me like a good use of maintainers' time.

If you want to make a genuine contribution, please have a look through projects' issues list to find out what changes they want.

Sure, But I am also suggesting my point of view in order to improve the efficiency and performance of the existing codebase. But I will focus more on the existing issues and understand your point of view.I request you to please change the label.

@Akshat111111
Copy link
Author

There is no reason to change docstrings to comments, or remove deprecations from functions. It makes it very difficult to determine what it is you actually intend here as an optimization. Please remove the superfluous changes so that it is clearer what is going on here. Also, did you run any benchmarks to show that these are indeed faster?

I agree with you, but I have gone through the documentation, and currently working on the performance optimization.But the rotation is actually simplified in accordance to local machine,But I am working more on it, in order to address it effectively.

@jklymak
Copy link
Member

jklymak commented Oct 26, 2023

If you work on it please submit a new PR with an explanation of the performance improvement ideally with evidence of an improvement using benchmarks. Note the pr should pass our tests or you should modify the test to show what changed

@Akshat111111
Copy link
Author

If you work on it please submit a new PR with an explanation of the performance improvement ideally with evidence of an improvement using benchmarks. Note the pr should pass our tests or you should modify the test to show what changed

Sure, I will do as you have instructed with a brief explanation and after effects, Please change the label of it please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants