Combinatorica 2.0

[rocky]

Personally, I am happy to see that we have made enough progress that a package I tried to get running a while ago and failed, now largely succeeds.

[TiagoCavalcante]

Wow, I'm surprised by this one. LGTM

[TiagoCavalcante]

Shouldn't only one version be loaded?

[rocky]

I think you are correct. Also we are loading in RSolve as well which is different.

I'll mark this as a draft then.

I don't understand how Mathics packaging works. What is intended is to load the package based on the name that is given in "Needs". For example if I write Needs["DiscreteMath`CombinatoricaV2.0`"] then only V2.0 should get loaded. But if I type Needs["DiscreteMath`CombinatoricaV0.6`"] then only V0.6 should get loaded.

Also, when I try test_combinatorica (for version 0.9) using the 2.0 version there is some code that fails. So right now 0.9 may work a little better for now.

Here are some other things that might be bugs.

Here are a couple of messages that I get when loading this unmodified:

In[1]:= Needs["DiscreteMath`CombinatoricaV2.0`"]
Set::write: Tag All in All::usage is Protected.
Syntax::com: Warning: comma encountered with no adjacent expression. The expression will be treated as Null (line 1266 of "/src/external-vcs/github/Mathics3/mathics-core/mathics/packages/DiscreteMath/CombinatoricaV2.0.m").

With respect to the Set::write message the system is basically saying that All:usage has already been defined which is why it is protected.

However the code has:

BeginPackage["DiscreteMath`Combinatorica2`"]
...
All::usage = "All is an option to inform certain functions to return all solutions, instead of just the first one."

So I would have expected All to be in the DiscreteMath`Combinatorica2 namespace rather than System.

As for: Syntax::com: Warning: comma encountered with no adjacent expression.,

we have:

		For [j=2, (j<n)  && (y[[j]]>=y[[j-1]]), j++, ];

and it doesn't like the trailing comma followed by nothing.

If you change that to:

		For [j=2, (j<n)  && (y[[j]]>=y[[j-1]]), j++, Null];

then the warning goes away.

@mmatera should there be a warning? And is there a bug in the namespacing in Mathics regarding the two "All"s

[mmatera]

Regarding All, we should check if it is not a bug of the package: it is possible that in an earlier version of WMA All were not a system symbol, or maybe it was not protected. On the other hand, it could happen that the BeginPackage mechanism in Mathics has a bug.

Regarding the For loop, in WMA, I got the warning: this is because the "body" of the For loop is missing.

[rocky]

So for the For loop, I will add back in Null since that is preferred.

For All, the question in my mind is less about whether WMA All was defined at the time the package was written but rather whether BeginPackage should allow a symbol that had been defined in System to get defined in the context of the package without getting that protected message.

If we wanted to handle the situation that All wasn't defined in Mathematica version 5 but is in Version 6, then we would write a Version 5 compatibility package that removes All.

[mmatera]

@rocky: regarding the warning on setting All::usage, it seems that in WMA, it is possible to assign messages to protected symbols. I am going to add a PR changing that here.

[rocky]

Ok - thanks. However the question I have is about the namespace aspect. Is setting All::usage setting System`All::usage or DiscreteMath`Combinatorica2`All::usage? If this is the case, the bug feels more in the category of Mathics not separating namespaces correctly.

[mmatera]

@rocky, the answer to this, as well the fix for the assignment module are in the PR #283.

Inside a Begin/End block or in a BeginPackage/EndPackage block, symbols in the context "System"` are accessible, so

BeginPackage["acontext`"]
None::usage= "someusage"
Begin[`implementation`]
All::usage= "someusage"
End[]
EndPackage[]

sets the usage message for System`All and System`None. So, in resume, Mathics is handling scoping in the same way that WMA.

[mmatera]

@rocky, this has worked without any modification?

[rocky]

@rocky, this has worked without any modification?

Actually, I changed things for the warnings mentioned above. However I undid the modifications and we get again the warning. However the test still works.

But for the more expanded set of tests in 0.9 there is at least one problem.

So in sum, there is measurable progress, although there is more work to be done.

[mmatera]

@rocky, I started to review this. The first thing that I found problematic is the definition of PermutationQ:

PermutationQ[p_List] := (Sort[p] == Range[Length[p]])

Suppose we set p=Permutations[Range[3]]
then, p evaluates to {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}}.

Sort[p] does not do anything, since the list is already lexicographically sorted. On the other hand,
Range[Length[p]] evaluates to {1,2,3,4,5,6}

The right implementation should be something like

PermutationQ[p_List] := Module[{sorted=p[[1]]}, And@@Table[sorted==Sort[el],{el, p}]]

[rocky]

Is this a change to Mathics or Combinatorica? If our Mathics code then please put in a PR for this. Thanks.

[mmatera]

@rocky, this is the line that I was talking about.

[rocky]

@rocky, this is the line that I was talking about.

Then I think there is a bug in Mathics not CombinatoricaV2.0.m. I checked with the published book, and indeed that code appears in section 2.1 on page 55.

When you run this on Mathematica, do you get the same behavior as Mathics?

[mmatera]

@rocky, this is the line that I was talking about.

Then I think there is a bug in Mathics not CombinatoricaV2.0.m. I checked with the published book, and indeed that code appears in section 2.1 on page 55.

When you run this on Mathematica, do you get the same behavior as Mathics?

The behaviour is the same for this definition (I changed the name to avoid a conflict with the builtin symbol):

In WMA:

In[1]:= MyPermutationQ[p_List]:=(Sort[p] == Range[Length[p]])                   

In[2]:= Permutations[Range[3]]                                                  

Out[2]= {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}}

In[3]:= ptst=Permutations[Range[3]]                                             

Out[3]= {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}}

In[4]:= MyPermutationQ[ptst]                                                    

Out[4]= {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}} == 
 
>    {1, 2, 3, 4, 5, 6}

In Mathics:

In[1]:= MyPermutationQ[p_List]:=(Sort[p] == Range[Length[p]])
Out[1]= None

In[2]:= Permutations[Range[3]]
Out[2]= {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}}

In[3]:= ptst=Permutations[Range[3]]                                             
Out[3]= {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}}

In[4]:= MyPermutationQ[ptst]
Out[4]= {{1, 2, 3}, {1, 3, 2}, {2, 1, 3}, {2, 3, 1}, {3, 1, 2}, {3, 2, 1}} == {1, 2, 3, 4, 5, 6}

In both cases, the result is not the expected (True)

[rocky]

Ok. Then I will contact the authors about this, and we can change what is in this branch adding a comment.

The book says this was uses Mathematica 4.2. Apparently things have changed since then.

[mmatera]

Or maybe I just do not understand what they were trying to do with this. The behavior of the involved symbols didn't change too much since v4.