Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Probability Distribution and Statistical Functions -- Normal Distribution Module #273

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

Merged
jvdp1 merged 57 commits into fortran-lang:master from Jim-215-Fisher:Distribution-Normal
Dec 9, 2021

Conversation

@Jim-215-Fisher
Copy link
Contributor

@Jim-215-Fisher Jim-215-Fisher commented Dec 21, 2020

This is the second round of probability distribution and statistical functions, continuing from #240. Since the whole modular structure has been changed, each distribution will have its own PR.
This PR contains normal distributions. Files uploaded are:
src/stdlib_stats_distribtuion_normal.fypp
src/CmakeLists.txt
src/Makefile.manual
src/tests/test_distribution_normal.f90
src/tests/CmakeLists.txt
src/tests/Makefile.manual
doc/specs/stdlib_stats_distribution_normal.md
doc/specs/index.md

@Jim-215-Fisher Jim-215-Fisher changed the title (削除) Probability Distribution and Statistical Functions -- Normal Module (削除ここまで) (追記) Probability Distribution and Statistical Functions -- Normal Distribution Module (追記ここまで) Dec 21, 2020
Copy link
Member

ivan-pi commented Dec 27, 2020

Hello @Jim-215-Fisher ,

it's an impressive undertaking what you have here!

I just gave the files a quick glance and noticed certain calls to real(val) might not be what was desired. To preserve the precision of the real part of a complex number you need to use the full syntax: real(cval,kind= ...). An alternative is to use cval%re. A discussion of this pitfall can be found here.

Copy link
Contributor Author

Jim-215-Fisher commented Dec 28, 2020
edited
Loading

Hello @Jim-215-Fisher ,

it's an impressive undertaking what you have here!

I just gave the files a quick glance and noticed certain calls to real(val) might not be what was desired. To preserve the precision of the real part of a complex number you need to use the full syntax: real(cval,kind= ...). An alternative is to use cval%re. A discussion of this pitfall can be found here.

Thanks for the point. I will take a look of it. Originally, I was using cval%re and cval%im, but it did not pass the compilation on ubuntu 7,8,9.

Copy link
Contributor Author

Hello @Jim-215-Fisher ,

it's an impressive undertaking what you have here!

I just gave the files a quick glance and noticed certain calls to real(val) might not be what was desired. To preserve the precision of the real part of a complex number you need to use the full syntax: real(cval,kind= ...). An alternative is to use cval%re. A discussion of this pitfall can be found here.

I checked the Fortran 2018 Handbook, it states like this for real(A, [KIND]):
"if A is type complex and KIND is not present, the kind type parameter is the kind type parameter of A."
This is the result I desired in the code. Maybe I missed something.

Copy link
Member

ivan-pi commented Dec 28, 2020

I checked the Fortran 2018 Handbook, it states like this for real(A, [KIND]):
"if A is type complex and KIND is not present, the kind type parameter is the kind type parameter of A."
This is the result I desired in the code. Maybe I missed something.

In that case consider my comment void. I was basing my comment of the comments I read on Fortran Discourse.

Copy link
Contributor Author

@jvdp1 @awvwgk @gareth-nx , the files have been updated. Especially documentation for complex variable case was changed.

jvdp1, awvwgk, and gareth-nx reacted with thumbs up emoji

Copy link
Contributor Author

@jvdp1 @awvwgk @gareth-nx All precision for probability were added according to @gareth-nx . Once this PR is done, I will set up a new PR for uniform distribution to correct similar problems.

gareth-nx reacted with thumbs up emoji

Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

Thankyou! This is a very important contribution to stdlib.


### Return value

The result is a scalar or rank one array, with a size of `array_size`, of type `real` or `complex`.
Copy link
Contributor

@gareth-nx gareth-nx Dec 5, 2021

Choose a reason for hiding this comment

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

I suggest also mentioning that the internal computations produce double precision random numbers, so higher precision outputs are simply type-cast from these and will not have full-precision accuracy.

Copy link
Member

jvdp1 commented Dec 7, 2021

@Jim-215-Fisher @gareth-nx is this PR ready for merging?

Copy link
Contributor

@jvdp1 @Jim-215-Fisher I think it just needs some minor documentation fixes mentioned above (or, a comment about why they are not needed -- I haven't looked in detail since reviewing). I was intending to approve once those points were addressed.

Copy link
Contributor Author

@Jim-215-Fisher @gareth-nx is this PR ready for merging?

I think it is ready.

Copy link
Contributor Author

@jvdp1 @Jim-215-Fisher I think it just needs some minor documentation fixes mentioned above (or, a comment about why they are not needed -- I haven't looked in detail since reviewing). I was intending to approve once those points were addressed.

I don't think we mention precision in documentation though. By default, all precision are supported.

Copy link
Contributor

@jvdp1 @Jim-215-Fisher I think it just needs some minor documentation fixes mentioned above (or, a comment about why they are not needed -- I haven't looked in detail since reviewing). I was intending to approve once those points were addressed.

I don't think we mention precision in documentation though. By default, all precision are supported.

Ahh, I see. I am getting confused between type and kind in Fortran.

So at the moment the documentation indicates the output type, but not the kind, and it is assumed to be implied that all kinds are supported.

I think it would be nicer to be more explicit about this. The same point was made in this review comment by @jvdp1 on October 10. @Jim-215-Fisher would you mind to use phrasing like this when describing the output variables?

Copy link
Contributor Author

@jvdp1 @Jim-215-Fisher I think it just needs some minor documentation fixes mentioned above (or, a comment about why they are not needed -- I haven't looked in detail since reviewing). I was intending to approve once those points were addressed.

I don't think we mention precision in documentation though. By default, all precision are supported.

Ahh, I see. I am getting confused between type and kind in Fortran.

So at the moment the documentation indicates the output type, but not the kind, and it is assumed to be implied that all kinds are supported.

I think it would be nicer to be more explicit about this. The same point was made in this review comment by @jvdp1 on October 10. @Jim-215-Fisher would you mind to use phrasing like this when describing the output variables?

done.

Copy link
Contributor

@gareth-nx gareth-nx left a comment

Choose a reason for hiding this comment

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

Many thanks!

Copy link
Contributor

@jvdp1 This is ready to merge from my view.

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. Thank you @Jim-215-Fisher for this and to all reviewers. i'll merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jvdp1 jvdp1 jvdp1 approved these changes

@awvwgk awvwgk awvwgk approved these changes

@gareth-nx gareth-nx gareth-nx approved these changes

Assignees

No one assigned

Labels

reviewers needed This patch requires extra eyes topic: statistics Statistical functions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /