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

Feature#12217 Enumerable/Array sum #4297

Merged
merged 6 commits into from Nov 15, 2016
Merged

Feature#12217 Enumerable/Array sum #4297

merged 6 commits into from Nov 15, 2016

Conversation

phluid61
Copy link
Contributor

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
@enebo
Copy link
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
Copy link
Contributor Author

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 feature/12217-enumerable-sum branch November 15, 2016 23:20
@enebo
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

Successfully merging this pull request may close these issues.

None yet

2 participants