On Sun, Feb 09, 2025 at 11:24:12AM +0000, Martin Baker wrote:
> Yes, I agree, it doesn't look correct.
>
> Let me look into it as my name is on the file as author.
>
> Its an interesting case study for trying to understand my own code after
> almost 10 years. I think I should have made the comments more verbose
> although I suspect Waldek would disagree?
Well, sometimes things need explanation. But in most cases when
I need a lot of words to explain something, this indicates that
my thinking is not clear enough.
> On 09/02/2025 09:33, Hill Strong wrote:
> > In the following function, the line row = row + 1 does not appear to be
> > correct
> >
> > isArrow?(arr : List(List(Boolean)), a : NNI, b : NNI) : Boolean ==
> > row : NNI := 1
> > for x in arr repeat
> > if row = a then
> > val : Boolean := qelt(x, b)
> > return val
> > row = row + 1
> > false
> >
> > Looking at the code, it would appear that this line should be
> >
> > row := row + 1
One question is if this code could ever produce intended result.
I think that it works when only first row is non-tivial. Testing
with more then one row should have found the trouble.
Second question is why this code is inside a category? This code
clearly assumes specific representation, so is more appropriate as
part of a domain.
Third question is why the specific representation that you use?
List allows sparse representation, but cases when one of rows is
absent are rather special. OTOH you use indexing on rows, which
forces dense representation. So, List(Boolean) for rows is both
less efficient computationally and takes more space than a vector.
Or to put is differently, code in category should do things independent
of representation. Specific domain could use dense matrix or
sparse matrix or list depending on what is appropriate. Or
possibly use code (an "oracle").
Concerning fixit it, using ':=' should correct functionality.
Better style would be
isArrow?(arr : List(List(Boolean)), a : NNI, b : NNI) : Boolean ==
for x in arr for row in 1.. repeat
if row = a then
return qelt(x, b)
false
There is also question of naming, using 'row' or maybe 'rl' instead of
'x' looks clearer. Of course, then you would need someting like 'rn'
or 'row_number'.
There is a question what should happen when b is out of range?
Currently code returns false when it can not find row, but
when row is present out of range b will give indexing error.
In fact, if you accept error for out of range row number you can
have
isArrow?(arr : List(List(Boolean)), a : NNI, b : NNI) : Boolean ==
qelt(qelt(arr, a), b)
OTOH if you want false instead of error you should add inner loop
instead of 'qelt' in original version. Or possibly simpler
but slightly less efficient:
isArrow?(arr : List(List(Boolean)), a : NNI, b : NNI) : Boolean ==
#arr < a => false
row := qelt(arr, a)
#row < b => false
qelt(row, b)
AFAICS there is similar trouble in 'setArrow!'.
To say the truth, before trying to fix the functions one should ask
if this code is ever used? Function in categories are only used via
inheritance to domains. If a domain provides its own representation,
then it probably also provides all functions depeneding on representation
(doing otherwise risks bugs when code changes). So possibly
simplest fix would just remove the problematic code.
--
Waldek Hebisch
--
You received this message because you are subscribed to the Google Groups
"FriCAS - computer algebra system" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to fricas-devel+unsubscr...@googlegroups.com.
To view this discussion visit
https://groups.google.com/d/msgid/fricas-devel/Z6img0j2ZpDrXVLw%40fricas.org.
