method to set a particular entry in a matrix to a value

[Alex-Jordan]

This is one of the methods that was planned in #1076, but not yet implemented. If $A is a matrix, then for example

$A->set(7, [1, 2]);

replaces $A with the same matrix, except the (1,2) position is replaced with 7. The 7 could be Real, Complex, or Fraction if we are in the appropriate ambient context.

1. I'm not sure the way that I am mutating $self for this is the best. @dpvc, do you have a better suggestion?
2. This doesn't work for a matrix of formulas (or is it a formula that returns a matrix?). For example, $A->set(Formula('x'), [1, 2]); returns an error that set is not a method for formulas. Does that mean this approach needs to be scrapped for something more robust?
3. I need to add unit testing. I'm waiting for feedback on 1 and 2 first.

[dpvc]

This last version is a significant improvement over your original one. But I do think it is a bit aggressive to tear down the entire matrix data structure and build a completely new one when all you want to do is replace one entry. It seems more efficient just to walk into the existing structure and replace the one entry.

Another issue is that it doesn't deal with some error conditions very well. For example, if you give a column index that is too large, you will get a message about rows needing to be the same size (perhaps not the most clear message for that), but if you give a row that is too large, you get no error, and the matrix is unchanged. Also, if you give an index of 0, you get the last entry, and a -1 gets the second to last, and so on. This is non-intuitive, at best.

Here is a version that doesn't replace the matrix data but instead just replaces the one entry, and that issues errors about incorrect indices, and so on. It doesn't allow negative indices, but you could add support for that if you want (but I would make -1 be the last row or column, and disallow a 0 index).

sub set {
  my ($self, $value, $indices) = @_;
  $value = Value::makeValue($value) unless Value::isValue($value);
  my @indices = @$indices;
  my $dim = scalar($self->dimensions);
  $self->Error("There should be $dim indices") unless $dim == @indices;
  my $data = $self->{data};
  my $i = pop(@indices) - 1;
  for (my $n = 0; @indices; $n++) {
    my $j = shift(@indices) - 1;
    $self->Error("The " . $self->NameForNumber($n) . " index is outside of the array bounds")
      if $j < 0 || $j >= scalar(@$data);
    $data = $data->[$j]->{data};
  }
  $self->Error("The last index is outside of the array bounds")
    if $i < 0 || $i >= scalar(@$data);
  $self->Error("The new entry value should be " . $data->[$i]->showType . " not " . $value->showType)
    unless $value->type eq $data->[$i]->type;
  my $old = $data->[$i];
  $data->[$i] = $value;
  return $old;
}

This also has the added effect of returning the entry that was originally in the matrix. I thought that might be useful, but you can remove the two lines concerning $old if you don't think so.

[Alex-Jordan]

Thanks @dpv, although I can't keep committing your work under my name :) I will change to this code, and attribute the commit to you.

I like the extra feature of returning the replaced value.

[dpvc]

I can't keep committing your work under my name

Sure you can. I'm happy not to have to go through the PR process myself (I do enough of that for MathJax).

I don't care about attribution for these things. I'm sorry if I'm being too pushy with my code. In th future, I could just suggest ideas rather than providing code; but I usually test out my ideas before presenting them, and that often means working out the details, and it seems a shame to then make someone else go through the same work all over again.

I'm happy to handle things however you all want, though.

[drgrice1]

Any way that good code that works well gets in is good. I agree that if you have worked out the details, then there is certainly no point in making someone else do it again. And @Alex-Jordan is always good with attribution too (better than most of the rest of us I think).

[Alex-Jordan]

I added a unit test for this.

[dpvc]

Should there be tests for the error conditions as well?

[dpvc]

Should this be set(9, [2,1])?

[Alex-Jordan]

OK, in there now too. I'm not sure how exhaustive to be. Like testing for an index that is too high, but also for one too low. Or testing the first index, a middle index, and the last index. I just went with a representative subset.

[Alex-Jordan]

No, I made it so you can pass the indices together as an array reference, or just let them be additional trailing inputs. So this test at line 227 should be identical to the test at line 225, just using different syntax. And checking the alternate syntax works.

[Alex-Jordan]

Sorry, I think I referenced the wrong line numbers. Should have wrote 226 and 228.

[dpvc]

No, I made it so you can pass the indices together as an array reference, or just let them be additional trailing inputs.

OK, I didn't notice that (I hadn't looked closely at the code, I'm afraid). Good to know.

[Alex-Jordan]

Yeah, I'm not sure it's a good idea. But it's what motivated putting the new entry as the first argument. I think it is more natural in English to say "replace the entry at (1,2) with 3" than to say "overwrite 3 into the entry at (1,2)". But for this purpose of allowing the indices to not be inside an array ref, I made it that way.

I suppose I could have used pop to keep the new entry at the end of the input array.

Or just not offer this flexibility.