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 logspace constructor #617

Merged
merged 5 commits into from May 4, 2019
Merged

Add logspace constructor #617

merged 5 commits into from May 4, 2019

Conversation

JP-Ellis
Copy link
Contributor

This pull requests adds the logspace constructor for Array1 that works in the same way as linspace but creates logarithmically spaced floats.

This implementation supports both positive and negative intervals, though
(obviously) not intervals which cross 0.0.

Signed-off-by: JP-Ellis <josh@jpellis.me>
@LukeMathWalker
Copy link
Member

Thanks for working on this @JP-Ellis!
It definitely looks like a piece of functionality that we were missing.

To move this forward it would be nice to have tests for these new functions, testing both happy and unhappy paths 👍

Signed-off-by: JP-Ellis <josh@jpellis.me>
@JP-Ellis
Copy link
Contributor Author

@LukeMathWalker All done, I have added tests within a tests submodule of the new logspace module.

Signed-off-by: JP-Ellis <josh@jpellis.me>
Signed-off-by: JP-Ellis <josh@jpellis.me>
@jturner314
Copy link
Member

I haven't had a chance to review this in detail, but at first glance, I had a couple of thoughts:

  • We should name this geomspace instead of logspace to be consistent with NumPy and Matlab. (logspace in NumPy and Matlab takes the exponent bounds as parameters, while geomspace in NumPy takes the actual value bounds as parameters, like this PR.)

  • I'm interested that the implementation of this PR multiplies by a step for each element. In contrast, NumPy's geomspace is implemented in terms of logspace, which is implemented by using linspace as an exponent. I would guess that directly computing base ^ (log_start + log_step * i) for each element (like NumPy) would be more accurate than (base ^ log_start) * step * step * ⋯ * step (this PR) due to accumulated errors, but I'm not really sure. There's also some advantage to maintaining consistency with NumPy, but that's not necessarily important. Any thoughts on this?

@LukeMathWalker
Copy link
Member

  • We should name this geomspace instead of logspace to be consistent with NumPy and Matlab. (logspace in NumPy and Matlab takes the exponent bounds as parameters, while geomspace in NumPy takes the actual value bounds as parameters, like this PR.)

If we decide to keep compatibility with their naming convention, then I'd advice to add a logspace-like method to our API as well. geomspace picks its name from geometric progression, but for searchability purposes I think most people looking for such a functionality (and I put myself in there) would expect something named after a logarithm, like logspace. If we provide both, I think we should be fine.

  • I'm interested that the implementation of this PR multiplies by a step for each element. In contrast, NumPy's geomspace is implemented in terms of logspace, which is implemented by using linspace as an exponent. I would guess that directly computing base ^ (log_start + log_step * i) for each element (like NumPy) would be more accurate than (base ^ log_start) * step * step * ⋯ * step (this PR) due to accumulated errors, but I'm not really sure. There's also some advantage to maintaining consistency with NumPy, but that's not necessarily important. Any thoughts on this?

I have the same gut feeling that base ^ (log_start + log_step * i) would lead to a more precise estimate, but no theoretical/practical example to back it up. We could run a quick check comparing errors for an (long) exact sequence.

Signed-off-by: JP-Ellis <josh@jpellis.me>
@JP-Ellis
Copy link
Contributor Author

JP-Ellis commented May 1, 2019

Great, so I've renamed the logspace function to geomspace, and I've added a new logspace function to be in line with NumPy and Matlab.

Additionally, I've changed the way each step is calculated to avoid a possible small error from accumulating.

Before merging this, I would recommend that this pull request be squashed into one commit.

@LukeMathWalker
Copy link
Member

It looks good to me - happy to squash and merge 🚀

@LukeMathWalker LukeMathWalker merged commit 3656ddf into rust-ndarray:master May 4, 2019
@JP-Ellis JP-Ellis deleted the logspace branch May 5, 2019 01:29
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