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

convert pdf_norm and cdf_norm to pure while improving scale check #679

Merged
merged 8 commits into from Mar 4, 2023

Conversation

HugoMVale
Copy link
Contributor

Issue to be solved
Procedures pdf_norm and cdf_norm of module stdlib_stats_distribution_normal are defined as impure because they include a call to the custom function error_stop if scale==0. This leads to the following problems/limitations:

  1. Being impure, these functions can't be used in parallel applications.
  2. There is no check if scale<0.
  3. If, in a given parallel process, array evaluation. etc., a single function call is invalid the whole calculation process is stopped.

Solution proposed

  1. Extend check scale==0 to scale<=0.
  2. If scale <=0, return NaN rather than calling error_stop(msg). Note: replacing error_stop(msg) by error stop msg would solve problem 1, but would not solve problem 3.
  3. Upgrade said functions to pure.

Note: The same approach can probably be used to other functions in this module, but I will wait for feedback before "expanding" the approach.

@HugoMVale HugoMVale changed the title convert pdf_norm and cdf_norm to pure while improve scale check convert pdf_norm and cdf_norm to pure while improving scale check Sep 16, 2022
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. To minimize the diff introduced, use 4 spaces for indentation in all stdlib routines.

@awvwgk awvwgk added the reviewers needed This patch requires extra eyes label Sep 21, 2022
@HugoMVale
Copy link
Contributor Author

Thanks for sharing. To minimize the diff introduced, use 4 spaces for indentation in all stdlib routines.

Thanks for the hint. I did as suggested: it helps a bit, but it still looks I changed almost the whole file...
If there is some additional formatting standard to comply to, just let me know.

!
! Latest version - 1 January 2001
!
impure subroutine zigset
Copy link
Member

Choose a reason for hiding this comment

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

This impure attribute is redundant. I would suggest to remove it in this case and other occurrences.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That deserves a bit of explanation. IMO, the attribute is not redundant from a "documentation" perspective.
A function without an explicit attribute will be treated as impure, but it may in reality be pure or impure. The only way to find out it is to look inside the code. Adding the impure statement there is meant to explicitly indicate that that function is really impure (it changes global variables).

Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay for impure to stay for documentation purposes.

@jvdp1
Copy link
Member

jvdp1 commented Sep 21, 2022

thank you @HugoMVale for this PR.
Two comments:

  • Could you use the orignal format, please (without re-formatting it)? This will facilitate the review of the actual changes. Re-formatting should be done in another PR, without changing the content.
  • Should the same changes be applied to the modules stdlib_stats_distribution_exponential and stdlib_stats_distribution_uniform?

@awvwgk
Copy link
Member

awvwgk commented Sep 21, 2022

Could you use the orignal format, please (without re-formatting it)? This will facilitate the review of the actual changes. Re-formatting should be done in another PR, without changing the content.

You can add the w=1 attribute to the URL and ignore (most) whitespace changes in the review for now: https://github.com/fortran-lang/stdlib/pull/679/files?w=1.

@HugoMVale
Copy link
Contributor Author

thank you @HugoMVale for this PR. Two comments:

  • Could you use the orignal format, please (without re-formatting it)? This will facilitate the review of the actual changes. Re-formatting should be done in another PR, without changing the content.
  • Should the same changes be applied to the modules stdlib_stats_distribution_exponential and stdlib_stats_distribution_uniform?
  1. I would gladly use the original format, if I knew what it was... In the code review window, there is an option to hide spaces.
  2. I would consider doing the same changes to the other two modules, but only after we agree that these changes are adequate (to avoid going back and forth).

@milancurcic
Copy link
Member

@HugoMVale, thanks for this PR. I've reviewed it (thanks @awvwgk for the w=1 trick! I didn't know about it). Yes, if you want, please go ahead with making the same changes to the other two modules. Though not preferred to mix formatting changes with feature/bugfix changes in the same PR, I don't think it's a big deal. Let's just make your work as easy as possible and move forward.

@milancurcic
Copy link
Member

I can't seem to be able to re-run the docs build job and see if it persistently fails (maybe there's a timeout period after which jobs can't be re-run). We'd need to fix this before merging either way.

@HugoMVale
Copy link
Contributor Author

I can't seem to be able to re-run the docs build job and see if it persistently fails (maybe there's a timeout period after which jobs can't be re-run). We'd need to fix this before merging either way.

I can't understand the origin of the problem from the build log. In principle, it can't be caused by the PR itself, right?

@milancurcic
Copy link
Member

It's here:

https://github.com/fortran-lang/stdlib/actions/runs/3100301687/jobs/5020886272#step:6:43

I don't know which submodule exactly it's coming from yet. I'll try to reproduce locally.

@14NGiestas
Copy link
Member

Hi, I believe you'll need to pick this PR in order fix the docs issue.

#681

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants