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

First implementation of real-valued linspace. #420

Merged
merged 94 commits into from
Jul 20, 2021

Conversation

ejovo13
Copy link
Contributor

@ejovo13 ejovo13 commented May 31, 2021

Hi all,

This is my first time contributing on any project, ever, so bear with me if I make any missteps. As per issue #17, there seems to be a demand to implement some simple functions that return a range of values.

So, I implemented linspace using MATLAB's syntax as a guide. Currently there is no support for passing complex values into the function but if this pull request gets approved I will implement it ASAP. This pull request is a gentle attempt to make my first contribution and to understand the ropes before I start trying to tackle other open issues.

I didn't know how to implement a robust test for checking all of the elements in the returned rank 1 array, so I instead check the first and last values, and the length of the returned array in the test_linspace.f90 test.

Nevertheless, here is a quick demo when using the linspace function from another project which imports the fortran_stdlib:

image

@awvwgk awvwgk added idea Proposition of an idea and opening an issue to discuss it reviewers needed This patch requires extra eyes and removed idea Proposition of an idea and opening an issue to discuss it labels May 31, 2021
Copy link
Member

@jvdp1 jvdp1 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 and welcome!
A general comment: I wonder if such a procedure would find a better place in the module stdlib_linalg.
In Matlab it is in the same section as eye, diag,....

src/stdlib_stats.fypp Outdated Show resolved Hide resolved
@milancurcic
Copy link
Member

Thank you @ejovo13!

I wonder if such a procedure would find a better place in the module stdlib_linalg.

There's also stdlib_math which I think is more appropriate.

@ejovo13
Copy link
Contributor Author

ejovo13 commented May 31, 2021

As per @milancurcic 's advice, I refactored the linspace interface and implementation under the stdlib_math module.
As per @jvdp1 and @arjenmarkus 's guidance, I removed the allocatable attribute from the result array and broke the function up into two different signatures, one where you pass the length of the array and the other where linspace will use the module's private paramater DEFAULT_LINSPACE_LENGTH, currently set to 100

@ejovo13
Copy link
Contributor Author

ejovo13 commented May 31, 2021

Hello. I've implemented the functions logspace and lnspace, defined in submodules stdlib_math_logspace and stdlib_math_lnspace.

logspace

The logspace function mimicks the matlab implementation. The first two parameters are used to build the range from base^start to base^end. When base is not specified, use base 10. When the length, n, is not specified, output a rank 1 array of 50 elements.

The various function signatures of logspace (and linspace) are staggered and not using optional parameters for two reasons. The first is that using the length n as an optional parameter forces us to use allocatable arrays as our result, which should be avoided. Secondly, when using the base as an optional argument, the generic procedure is unable to distinguish function calls when the base is not passed.

this logspace implementation differs from MATLAB's because we can choose the base with which to generate the logarithmic space.

lnspace

lnspace is a special case of logspace when the base is equal to Euler's number e. Various levels of precision of EULERS_NUMBER are used in the stdlib_math module.

Tests

Running make test will check certain qualities about the results and will output a few select log files showing the arrays generated from their calls for manual inspection. Here is a gist that shows sample output.

@arjenmarkus
Copy link
Member

arjenmarkus commented Jun 1, 2021 via email

ejovo13 and others added 6 commits July 12, 2021 13:55
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@awvwgk awvwgk dismissed their stale review July 12, 2021 20:23

Requested changes addressed

src/stdlib_math.fypp Outdated Show resolved Hide resolved
ejovo13 and others added 2 commits July 12, 2021 16:37
Demand a lower tolerance value for higher precision tests
Co-authored-by: Sebastian Ehlert <28669218+awvwgk@users.noreply.github.com>
@ejovo13
Copy link
Contributor Author

ejovo13 commented Jul 12, 2021

Well, I believe that I've addressed, on my side, all of your comments. What I thought was going to be a quick and easy first PR has dragged on (not that that's a bad thing) and generated a lot of fruitful discussion. Thank you to everyone for their contributions and suggestions to improve this PR.

Apart from a few unresolved discussions, I don't believe that there is anything left for me to do. The ball is in your court and I will wait for any further direction. Thank you all.

@ejovo13 ejovo13 requested review from jvdp1 and awvwgk July 12, 2021 21:10
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

LGTM.
I approve it, pending a few minor comments.
Regarding the checks, I would prefer checks on the absolute values, instead of on relative values. But I leave that for the next reviewer.

doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
ejovo13 and others added 2 commits July 13, 2021 21:16
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

Pending any unresolved requests, this looks ready to be part of stdlib. Thanks a lot for sharing this contribution.

ejovo13 and others added 3 commits July 14, 2021 10:01
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
Co-authored-by: Jeremie Vandenplas <jeremie.vandenplas@gmail.com>
@jvdp1
Copy link
Member

jvdp1 commented Jul 14, 2021

@ivan-pi @milancurcic you commented this PR. Could you check if your comments were answered appropriately, and appove this PR if it is ok, please?

Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

Some details in the docs

src/stdlib_math.fypp Outdated Show resolved Hide resolved
src/stdlib_math.fypp Outdated Show resolved Hide resolved
Copy link
Member

@milancurcic milancurcic 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, great addition. I only left one nit-pick about labeled format statements in test subroutines.

src/tests/math/test_linspace.f90 Outdated Show resolved Hide resolved
@jvdp1 jvdp1 merged commit 233d871 into fortran-lang:master Jul 20, 2021
@ejovo13 ejovo13 deleted the lin_log_space branch July 20, 2021 20:17
@awvwgk awvwgk removed the reviewers needed This patch requires extra eyes label Sep 25, 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.

8 participants