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

Add generic pow method for arrays #1053

Closed
wants to merge 6 commits into from

Conversation

multimeric
Copy link

Closes #1047.

Originally I was going to make separate powi() and powf() methods, but this isn't convenient because numtraits groups them together using one method.

One open question is how to get an integer array to take a float power and vice-versa, because numtraits doesn't seem to implement this use-case.

I've written two tests, which are in the documentation. I hope that's sufficient.

@multimeric
Copy link
Author

multimeric commented Aug 3, 2021

@ maintainers, this seems to be failing because of something to do with #[inline], which I haven't touched. Is master broken or did this PR break it in a confusing way?

@jturner314
Copy link
Member

@ maintainers, this seems to be failing because of something to do with #[inline], which I haven't touched. Is master broken or did this PR break it in a confusing way?

master currently fails CI due to new warnings with Rust nightly. I have a fix in #1054, which I'll merge as soon as CI passes.

@multimeric
Copy link
Author

Great thanks. Hopefully this will rebuild automatically once that gets merged?

@jturner314
Copy link
Member

Great thanks. Hopefully this will rebuild automatically once that gets merged?

Once #1054 is merged, I think you'll need to fetch the latest master from GitHub, rebase the pow branch onto it, and then force-push pow to update this PR. Are you familiar with how to do that, or would you like assistance?

@multimeric
Copy link
Author

I think I can just pull master in my branch and then push my branch right?

@jturner314
Copy link
Member

I think I can just pull master in my branch and then push my branch right?

That works too. The only disadvantage is that the commit history won't be as pretty as with a rebase.

@multimeric
Copy link
Author

multimeric commented Aug 3, 2021

By the way, can I have some advice on the "error: unused import: Pow" I'm getting in the build? I am using that trait in my method, but I guess when we don't have the std feature enabled the method is skipped and so Pow seems to be unused?

@multimeric
Copy link
Author

That works too. The only disadvantage is that the commit history won't be as pretty as with a rebase.

Okay I've rebased.

@jturner314
Copy link
Member

jturner314 commented Aug 3, 2021

By the way, can I have some advice on the "error: unused import: Pow" I'm getting in the build? I am using that trait in my method, but I guess when we don't have the std feature enabled the method is skipped and so Pow seems to be unused?

Yes, that's the issue. The simplest fix would be move use num_traits::Pow; to a separate line and add #[cfg(feature = "std")]. Edit: Actually, just move Pow onto the same line as the use num_traits::Float import. Edit2: Great, it looks like you noticed that.

@multimeric
Copy link
Author

Yes, that's the issue. The simplest fix would be move use num_traits::Pow; to a separate line and add #[cfg(feature = "std")].

Done, thank you.

@multimeric
Copy link
Author

Damn, it looks like this is all duplicating a subset of #1042. I wish I knew about that!

@multimeric
Copy link
Author

Closing in favour of #1042.

@multimeric multimeric closed this Sep 30, 2021
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.

Implement powi and powf for ArrayBase
2 participants