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

Feature#12217 Enumerable/Array sum #4297

Merged
merged 6 commits into from Nov 15, 2016

Conversation

Projects
None yet
2 participants
@phluid61
Contributor

phluid61 commented Nov 15, 2016

Adds Enumerable#sum and Array#sum from Feature #12217.

I'm parking this PR here in its current state to encourage discussion and feedback.

@enebo enebo added this to the JRuby 9.2.0.0 milestone Nov 15, 2016

@enebo enebo merged commit 067cad1 into jruby:ruby-2.4 Nov 15, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@enebo

This comment has been minimized.

Member

enebo commented Nov 15, 2016

@phluid61 I do have two questions. I suspect there are MRI tests we will now pass but I did not try it out so I don't know. Is that true? Secondly, can you check to make sure the sub is really an optional arity for Enumerable.sum?

@phluid61

This comment has been minimized.

Contributor

phluid61 commented Nov 15, 2016

There are tests, yes:

I am in the process of porting them across, so I can run them and make sure it all passes.

Regarding the arity, I think so. That means that this is allowed, right?

~$ ruby -v -e 'p (-1..1).sum{1}'
ruby 2.4.0dev (2016-11-02 trunk 56546) [x86_64-linux]
3
~$ 

Or have I misunderstood what the arity means in that context in JRuby?

@phluid61 phluid61 deleted the phluid61:feature/12217-enumerable-sum branch Nov 15, 2016

@enebo

This comment has been minimized.

Member

enebo commented Nov 15, 2016

@phluid61 yeah that probably means it accepts any number of arguments. Zero would not work if it was required to be 1. sum { |a,b,c| 1 } should also work and I am sure it does based on that.

BTW we should also have those imported at test/mri/ruby/test_*.rb. We also can have excludes omitting tests in test/mri/excludes. You will see a name matching the class in those files with the method names which we will exclude (just letting you know as I do not see any excludes for test_sum).

@phluid61

This comment has been minimized.

Contributor

phluid61 commented Nov 16, 2016

I've got my jruby branch built and executing. There are problems:

  • array fails because of RbConfig::SIZEOF
~$ EXCLUDES=test/mri/excludes bin/jruby test/mri/runner.rb ruby/test_array.rb
/home/matty/ruby/jruby/test/mri/ruby/test_array.rb: no such file to load -- rbconfig/sizeof
  • enum fails because of rounding. I assume this is related to "Kahan's compensated summation algorithm" used/spec'd in MRI
[55/66] TestEnumerable#test_sum = 0.02 s                                        
  3) Failure:
TestEnumerable#test_sum [/home/matty/ruby/jruby/test/mri/ruby/test_enum.rb:903]:
<100000000.00000001> expected but was
<100000000.0>.
@phluid61

This comment has been minimized.

Contributor

phluid61 commented Nov 16, 2016

Well, that was ugly. I got MRI's enum tests to pass, and then I piggy-backed Array's implementation on it.

The spec/test coverage isn't astounding, but I'm pretty sure I understood the MRI implementation well enough to catch the edge-cases.

@phluid61

This comment has been minimized.

Contributor

phluid61 commented Nov 17, 2016

I completely rebuilt Array#sum to better match MRI's implementation.

Then I created my own fake rbconfig/sizeof.rb so I could run MRI's Array tests. Everything seems good now, but I can't test against 'mathn' because:

$ bin/jruby -rmathn
NameError: method '/' not defined in Integer
    remove_method at org/jruby/RubyModule.java:2884
  <class:Integer> at /home/eprints/ruby/jruby/lib/ruby/stdlib/mathn.rb:68
           <main> at /home/eprints/ruby/jruby/lib/ruby/stdlib/mathn.rb:67
          require at org/jruby/RubyKernel.java:961
           (root) at /home/eprints/ruby/jruby/lib/ruby/stdlib/rubygems/core_ext/kernel_require.rb:1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment