-
Couldn't load subscription status.
- Fork 196
convert pdf_norm and cdf_norm to pure while improving scale check
#679
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
Conversation
zigset changes global parameters and thus can't be pure
pdf_norm and cdf_norm to pure while improve scale check (削除ここまで)pdf_norm and cdf_norm to pure while improving scale check (追記ここまで)
There was a problem hiding this 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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
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_exponentialandstdlib_stats_distribution_uniform?
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.
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_exponentialandstdlib_stats_distribution_uniform?
- 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.
- 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).
@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.
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 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?
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.
Hi, I believe you'll need to pick this PR in order fix the docs issue.
Issue to be solved
Procedures
pdf_normandcdf_normof modulestdlib_stats_distribution_normalare defined asimpurebecause they include a call to the custom functionerror_stopif scale==0. This leads to the following problems/limitations:Solution proposed
error_stop(msg). Note: replacingerror_stop(msg)byerror stop msgwould solve problem 1, but would not solve problem 3.Note: The same approach can probably be used to other functions in this module, but I will wait for feedback before "expanding" the approach.