exercism / problem-specifications

Shared metadata for exercism exercises.
MIT License
327 stars 541 forks source link

all-your-base: Unary test cases #1520

Closed anupamsobti closed 2 years ago

anupamsobti commented 5 years ago

Currently, the unary case (base 1) of the all-your-base exercise considers base 1 as invalid. However, for base 1, one may represent the number as the length of output (with any symbol). Would you consider adding the appropriate tests?

petertseng commented 5 years ago

This comment neither recommends for nor against adding such tests (I have no recommendation in either direction), but provides three pieces of additional information that may help in making the decision.

  1. Although any symbol might be acceptable, I notice that consistently for other bases B digits d_i must be in the interval 0 <= d_i < B. Accordingly for base 1, it would be more consistent with other bases if only 0 was accepted as valid digit for that base. If not, base 1 would be a special case.
  2. The usual algorithm for calculating the value of a number that was expressed in a given base B would be to sum each digit d_i multiplied by an appropriate power of that base, \sum_{i=0}^n d_i B^i ( \sum_{i=0}^n d_i B^i ). If we take the above point that 0 is the only valid digit of base 1, there will instead need to be a special case for base 1, because otherwise surely the value of the above sum will be 0 since all d_i are 0. A possible alternative is to contravene the above convention of 0 <= d_i < B in some defined way.
  3. The usual algorithm for expressing a given value in a chosen base involves repeatedly dividing by that base. This implies the need for a special case for base 1, because dividing by 1 leaves the number unchanged.

Accordingly with the above pieces of information, the modifications I had to make to the existing code to support base 1 were the following diff:

diff --git i/exercises/all-your-base/verify.rb w/exercises/all-your-base/verify.rb
index 2c96d12..c624c17 100644
--- i/exercises/all-your-base/verify.rb
+++ w/exercises/all-your-base/verify.rb
@@ -5,10 +5,14 @@ json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))

 verify(json['cases'], property: 'rebase') { |i, _|
   base = i['inputBase']
-  raise "no base #{base}" if base < 2
-  i['digits'].reduce(0) { |a, e|
+  raise "no base #{base}" if base <= 0
+  val = i['digits'].reduce(0) { |a, e|
     raise "nope, #{e} negative" if e < 0
     raise "nope, #{e} too big" if e >= base
+    next a + 1 if base == 1
     a * base + e
-  }.digits(i['outputBase']).reverse
+  }
+
+  out_base = i['outputBase']
+  out_base == 1 ? [0] * val : val.digits(out_base).reverse
 }

Unless there is a clever change to make to the usual algorithms, one might expect student implementations to also have to add similar things if these test cases were added.

SleeplessByte commented 5 years ago

Unless there is a clever change to make to the usual algorithms, one might expect student implementations to also have to add similar things if these test cases were added.

We're not looking to make this exercise more difficult or complex, so following this reasoning I personally vote against adding base 1.

The required change seems minimal though. Everything is changed inline, and a special case is added at the bottom of the solution

+  i['outputBase'] == 1 ? [0] * val : val.digits(i['outputBase']).reverse
anupamsobti commented 5 years ago

I agree that from the perspective of the programming exercise, conforming to the general case seems to be a better option. Perhaps, we could add a note about it in the description, hence, ensuring that the exercise doesn't give rise to a mathematically incorrect assumption that base 1 is an invalid base.

SleeplessByte commented 5 years ago

@anupamsobti yes! That is always an option 👍. In that case I would suggest adding a section to the description along the lines of "although base 1 is considered [a valid base](url), for the sake of simplicity it's excluded in this exercise"

anupamsobti commented 5 years ago

Sounds good. Does this need to be taken care of in individual tracks or is there a common place for descriptions in this repository?

cmccandless commented 5 years ago

The exercise's description.md is used for generating the track exercise README files, so the change should be made there.

anupamsobti commented 5 years ago

Great. Shall I raise a PR?

SleeplessByte commented 5 years ago

@petertseng can you teach me (and others) how to use that tool you use to:

I couldn't find it in the docs -- hijacking this issue for now and then maybe add it via a PR to the docs. I'd like to apply your method whenever I merge something until we have the automatic-bot notifications Katrina talked about.

petertseng commented 5 years ago

figure out which tracks have an exercise

The high-level strategy is to:

As https://github.com/exercism/exercism/issues/4418 states, it doesn't seem like there is a better strategy available. Someone might choose to add it to the API if desired.

I wrote an implementation of the above two steps of the high-level strategy:

Of course, any other implementation of the strategy will serve equivalent purposes. There may be other implementations I do not know about.

create an issue tell them what's happened

It sounds like this is about what to write in the body of the issue rather than how to do it. https://github.com/exercism/docs/issues/10 was the documentation but the link is outdated. I found some equivalent documentation in https://github.com/exercism/docs/blob/master/you-can-help/improve-exercise-metadata.md#evolving-exercises . So that says what you should keep in mind when writing the issue.

I have just read it and all the advice in there still seems relevant.

copy it to all those tracks repositories

Here are some possible ways I know of to do this.

ErikSchierboom commented 2 years ago

@anupamsobti Are you still interested in working on this issue? If so, you could PR the change to https://github.com/exercism/problem-specifications/blob/main/exercises/all-your-base/canonical-data.json.

anupamsobti commented 2 years ago

I won't be able to contribute now. Thanks for asking.

ErikSchierboom commented 2 years ago

Okay thanks. Looking at @petertseng's example for what an implementation would look like, I'd say the benefits of adding these new test cases might be relatively compared to the "messiness" of the implementation. My vote would be to not add these test cases and close this issue. If anyone feels like they'd like this issue to remain open, let me know.