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 function diff #605

Merged
merged 5 commits into from
Jan 26, 2022
Merged

Conversation

zoziha
Copy link
Contributor

@zoziha zoziha commented Dec 19, 2021

  • Add diff: Differences and approximate derivatives

Description

Computes differences between adjacent elements of an array.

Note: Only diff for rank-1/rank-2 arrays at this stage. (If there is a higher rank demand in the future, we can consider extending it.)

API:
For a rank-1 array

y = [[stdlib_math(module):diff(interface)]](x [, n=1, prepend=<no value>, append=<no value>])

and for a rank-2 array

y = [[stdlib_math(module):diff(interface)]](x [, n=1, dim=1, prepend=<no value>, append=<no value>])
  • X: real/integer types.
  • n: Difference order.
  • ...

Note

Originally diff should be putted in stdlib_linalg, but considering that diff can support date drived types in the future, now it is putted in stdlib_math.

Prior Art

@zoziha zoziha changed the title Add diff [stdlib_math] Add function diff Dec 19, 2021
@awvwgk awvwgk added reviewers needed This patch requires extra eyes topic: mathematics linear algebra, sparse matrices, special functions, FFT, random numbers, statistics, ... labels Dec 19, 2021
@zoziha zoziha marked this pull request as draft December 20, 2021 02:20
@zoziha zoziha marked this pull request as ready for review December 20, 2021 03:06

#:for k1, t1 in RI_KINDS_TYPES
pure module function diff_1_${k1}$(X, n) result(Y)
${t1}$, intent(in) :: X(:)
Copy link
Contributor

@gareth-nx gareth-nx Dec 20, 2021

Choose a reason for hiding this comment

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

I don't know whether this interface has been discussed anywhere so far? A few potential issues that come to mind are:

  • This routine allocates its output on every call. I can imagine in some situations that could be inefficient. Users might wish to make a more efficient diff call by providing the output data. For that case a subroutine interface may be preferable.
  • When computing differences in practice, there are different approaches that may be desired to treat the end-points. The current routines reduce the size of the array. But one might also wish to preserve the array size and do some kind of extrapolation at the missing entry. There can be a number of cases here (e.g. forward differences, backward differences, centred differences, with different implications for end-points).

Currently the routine won't support such things. While that might prove to be fine, I'd like to see some discussion to ensure we've chosen the right interface.

@jvdp1
Copy link
Member

jvdp1 commented Dec 22, 2021

What is the state-of-the-art of such a function (e.g., how is it designed in other languages?)?

@zoziha zoziha linked an issue Dec 23, 2021 that may be closed by this pull request
@zoziha zoziha marked this pull request as draft December 28, 2021 04:31
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 for this PR. Here are some suggestions and comments.

@zoziha

This comment has been minimized.

@zoziha zoziha force-pushed the add_diff branch 2 times, most recently from cf3483a to bd52051 Compare January 2, 2022 06:26
@zoziha zoziha marked this pull request as ready for review January 2, 2022 06:37
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. Here are a couple of minor comments.

@zoziha
Copy link
Contributor Author

zoziha commented Jan 2, 2022

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. Pending a few comments, it could be merged IMO, Thank you @zoziha for this PR.

@zoziha
Copy link
Contributor Author

zoziha commented Jan 16, 2022

Thank you @jvdp1 for your patient review, I have made changes accordingly :)

@jvdp1
Copy link
Member

jvdp1 commented Jan 18, 2022

Thank you @zoziha. This PR is fine for me. If @gareth-nx or @awvwgk could have a final look, it would be nice ;)

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.

Looks good, thank you! I left several suggestions in the spec doc.

@zoziha
Copy link
Contributor Author

zoziha commented Jan 20, 2022

Thanks @milancurcic , I have made changes accordingly.
Since my English expressions may not be accurate, please point them out in time, thanks a lot.

@milancurcic
Copy link
Member

@gareth-nx @awvwgk are you OK with this PR going forward?

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.

Looks good to me.

@gareth-nx gareth-nx merged commit 6957436 into fortran-lang:master Jan 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewers needed This patch requires extra eyes topic: mathematics linear algebra, sparse matrices, special functions, FFT, random numbers, statistics, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion of diff procedure
5 participants