Skip to content

Conversation

aradi
Copy link
Member

@aradi aradi commented Jan 29, 2020

  • Substantially reduced code & number of fypp-constructs in mean().
  • Extended mean() to handle reduction from 1D-array to scalar (similar to sum())
  • Introduced tolerances when testing floating point results (note checking with == 0.0 is highly unreliable and may give false positives)
  • Reformatted test file according to style guide.

Commits in order of relevance, feel free to cherry pick if you don't like all of them.

@aradi aradi requested a review from jvdp1 January 29, 2020 08:44
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.

These improvements look really good to me (I tried to achieve a similar thing but without success; the reduce_shape is really useful). Thanks @aradi .

@aradi aradi requested a review from certik January 29, 2020 09:42
Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

Can you please submit the formatting changes in a separate PR? Let's not pollute our PRs with formatting.

Btw, the style guide says to use 4 spaces:

https://github.com/fortran-lang/stdlib/blob/11002bd4797b0fe015bf831a53f8794e32ea32b4/STYLE_GUIDE.md

Either way, that is irrelevant to the actual changes in this PR, which all look good to me. So if you could please split the formatting into a new PR, then let's get it PR merged.

@aradi
Copy link
Member Author

aradi commented Jan 29, 2020

Well, the styleguide says the body of every Fortran construct should be indented by two (4) spaces, so I have chosen the one I prefer more 😆 .

Anyway, I have taken out the reformatting commit, it really does not belong to here, I agree. I'll wait with reformatting changes until the style guide becomes non-ambigous. 😉

@certik
Copy link
Member

certik commented Jan 29, 2020

@aradi ha, that was a mistake, thanks for spotting it. I fixed it in #131.

@certik certik merged commit ecfbfb0 into fortran-lang:master Jan 29, 2020
@certik
Copy link
Member

certik commented Jan 29, 2020

@aradi the PR looks great, thank you!

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

Labels

None yet

3 participants