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

[stdlib_math] Add arange function. #480

Merged
merged 5 commits into from
Aug 9, 2021
Merged

[stdlib_math] Add arange function. #480

merged 5 commits into from
Aug 9, 2021

Conversation

zoziha
Copy link
Contributor

@zoziha zoziha commented Jul 31, 2021

Description

seq returns evenly spaced values within a given interval.

  • Add seq function. (see numpy/arange)
    (the API name seq needs to be discussed, arange? I think seq is better, it is shorter.)
    (aready to arange🔰)

Tasks

More routines to do, not this PR

see #476.

doc/specs/stdlib_math.md Outdated Show resolved Hide resolved
src/stdlib_math_seq.fypp Outdated Show resolved Hide resolved
@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label Jul 31, 2021
@awvwgk
Copy link
Member

awvwgk commented Jul 31, 2021

This looks like a useful addition, but the name seq seems rather short. Why not use arange here? sequence would also be an option, but this is already an attribute for derived types used for memory alignment or creating non-extendible types.

@zoziha

This comment has been minimized.

@zoziha zoziha changed the title [stdlib_math] Add seq function. [stdlib_math] Add arange function. Aug 3, 2021
Update related docs.
src/stdlib_math_arange.fypp Outdated Show resolved Hide resolved
src/stdlib_math_arange.fypp Outdated Show resolved Hide resolved
@Jim-215-Fisher
Copy link
Contributor

I will review this one.

doc/specs/stdlib_math.md Show resolved Hide resolved
src/stdlib_math_arange.fypp Outdated Show resolved Hide resolved
@zoziha
Copy link
Contributor Author

zoziha commented Aug 4, 2021

This doesn't sound right for the real values. How about arange(1.0,6.0,2.0) which should give us [1.0, 3.5, 6.0]. Here it gives us [1.0, 3.0, 5.0].

Thanks for your review! @Jim-215-Fisher arange is different with linspace, I think you may be talking about the function of stdlib_math(module):linspace(interface).
(see numpy: arange and linspace)

@Jim-215-Fisher
Copy link
Contributor

This doesn't sound right for the real values. How about arange(1.0,6.0,2.0) which should give us [1.0, 3.5, 6.0]. Here it gives us [1.0, 3.0, 5.0].

Thanks for your review! @Jim-215-Fisher arange is different with linspace, I think you may be talking about the function of stdlib_math(module):linspace(interface).
(see numpy: arange and linspace)

After reading numpy's documents, I understand that arange has a fixed spacing while linspace has fixed number of samples. The fixed number of samples give evenly spaced results. The fixed spacing in arange may give "evenly" spaced results without end point. My suggestion would be to change the wording of description for the function from "evenly spaced values" to "fix spaced values".

Another question is whether this function is elemental. If it is, why not change "pure" to "elemental"? In numpy, 'arange' is suitable to array arguments.

@zoziha

This comment has been minimized.

@Jim-215-Fisher
Copy link
Contributor

Another question is whether this function is elemental. If it is, why not change "pure" to "elemental"? In numpy, 'arange' is suitable to array arguments.

Thanks, fixed spaced values is indeed better.

This function is not elemental. It does not seem to meet the definition of elemental function.
I have not used numpy, but it seems that numpy.arange does not state that it supports array arguments.

Do you mean the return value is an array (not only a vector)? In addition, it is difficult for the existing fortran syntax to implement a multi-dimensional array generic interface. (see j3-fortran/fortran_proposals#153)

You are right. numpy.arange does not support array argument. Never mind.

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 for this PR! I think the API is mostly good, I gave some naming suggestions to use instead of by, and I think there is an issue with using present() inside merge(), so I suggested using optval() instead.

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
src/stdlib_math_arange.fypp Outdated Show resolved Hide resolved
src/stdlib_math_arange.fypp Outdated Show resolved Hide resolved
1. `merge` -> `optval` inside.
2. `by` -> `step` argument name.
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.

Thanks for sharing, looks good to me.

@milancurcic
Copy link
Member

With 3 approvals, I'll go ahead and merge. Thank you @zoziha and reviewers!

@milancurcic milancurcic merged commit 4052bd2 into fortran-lang:master Aug 9, 2021
@zoziha zoziha deleted the add_arange branch August 9, 2021 05:00
@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.

None yet

4 participants