ianhinder / Kranc

A Mathematica package for generating code for solving time dependent partial differential equations
http://kranccode.org
GNU General Public License v2.0
28 stars 10 forks source link

ValueQ change in Mathematica 12.2 breaks Kranc code generation in ET #151

Closed physicsbeany closed 2 years ago

physicsbeany commented 3 years ago

Today I tried using the Kranc-based McLachlan Mathematica scripts to regenerate the ML_BSSN thorn in the current stable release of the Einstein Toolkit, and it failed (error below). I'm running Mathematica 12.2.0 on a MacOS machine (10.15.7 -- Catalina). On reporting this to the Einstein Toolkit Users mailing list, and the failure was verified by Roland Haas.

On digging further, Roland found that Mathematica changed the behavior of the "ValueQ" function [ https://reference.wolfram.com/language/ref/ValueQ.html ] between v 12.1 and 12.2. The old behavior can be restored by specifying Method -> "Legacy" when invoking ValueQ in Kranc's TensorTools.m

===================== failure message from make =====================================
gs66-dodder:m bjkelly1$ make McLachlan_BSSN.out
rm -rf ML_BSSN ML_BSSN_Helper
env ML_CODE=BSSN ./runmath.sh McLachlan_BSSN.m McLachlan_BSSN
Using Kranc installation at ../../../repos/Kranc/Bin/..
Mathematica 12.2.0 Kernel for Mac OS X x86 (64-bit)
Copyright 1988-2020 Wolfram Research, Inc.
Profiling disabled

MapThread::mptd: Object TensorTools`Private`TensorCharacter[dir] at position {2, 2} in MapThread[NumberQ[#1] || NumberQ[#2] || #1 === #2 & , {{u}, TensorTools`Private`TensorCharacter[dir]}] has only 0 of required 1 dimensions.

MapThread::mptd: Object TensorTools`Private`TensorCharacter[epsdiss] at position {2, 2} in MapThread[NumberQ[#1] || NumberQ[#2] || #1 === #2 & , {{u}, TensorTools`Private`TensorCharacter[epsdiss]}] has only 0 of required 1 dimensions.

Riffle::list: List expected at position 1 in Riffle[TensorTools`Private`TensorCharacter[gt], ,].

StringJoin::string: String expected at position 1 in StringJoin[Riffle[TensorTools`Private`TensorCharacter[gt], ,]].

StringJoin::string: String expected at position 2 in Tensor indices in gt[la,lb] do not match those used previously: gt[<>Riffle[TensorTools`Private`TensorCharacter[gt], ,]<>].
Error:
StringJoin["Tensor indices in gt[la,lb] do not match those used previously: gt[", Riffle[TensorTools`Private`TensorCharacter[gt], ","], "]"]
make: *** [McLachlan_BSSN.out] Error 1
rhaas80 commented 3 years ago

This patch to Kranc makes it possible to generate ML_BSSN but is not backwards compatible since ValueQ in 12.1 does not take any Method argument.

diff --git a/Tools/CodeGen/TensorTools.m b/Tools/CodeGen/TensorTools.m
index 9e53a0a..1f17b35 100644
--- a/Tools/CodeGen/TensorTools.m
+++ b/Tools/CodeGen/TensorTools.m
@@ -301,7 +301,7 @@ DefineTensor[T_] :=
       Module[
         {c},
         c = tensorCharacter[{is}];
-        If[!ValueQ[TensorCharacter[T]],
+        If[!ValueQ[TensorCharacter[T], Method -> "Legacy"],
            TensorCharacter[T] = c,
            (* else *)
            If[!charactersMatch[c,TensorCharacter[T]],

Documentation for 12.1's ValueQ is here: https://reference.wolfram.com/legacy/language/v12.1/ref/ValueQ.html and indicates that the "old" method of ValueQ was checking of the result of evaluating FOO produced a result that differed from FOO.

ianhinder commented 3 years ago

You can add this option conditional on the Mathematica version. See, for example, SimulationTools init.m. Something like this might work:

 If[!ValueQ[TensorCharacter[T], If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]],
rhaas80 commented 3 years ago

You can add this option conditional on the Mathematica version. See, for example, SimulationTools init.m. Something like this might work:

 If[!ValueQ[TensorCharacter[T], If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]],

Unfortunately this does not seem to work. In 12.0 this returns ValueQ unevaluated. Eg:

Wolfram Language 12.0.0 Engine for Linux x86 (64-bit)
Copyright 1988-2019 Wolfram Research, Inc.

In[1]:= ValueQ[A, If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]]

Out[1]= ValueQ[A, If[$VersionNumber >= 12.2, Method -> Legacy, Sequence[]]]

In[2]:= If[ValueQ[A, If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]], Print["Value"], Print[
"No value"]]

Out[2]= If[ValueQ[A, If[$VersionNumber >= 12.2, Method -> Legacy, Sequence[]]], Print[Value],

>    Print[No value]]

so the condition would have to be:

If[$VersionNumber >= 12.2, ValueQ[A, Method -> "Legacy"], ValueQ[A]]

everywhere ValueQ occurs.

rhaas80 commented 3 years ago

What works in both versions seems to be to build one's own 12.0 look-alike ValueQ:

SetAttributes[MyValueQ, HoldAll]
MyValueQ[x_] := Not[x === Unevaluated[x]]

T[a] = 1

Print["ValueQ", ValueQ[T[a]], ValueQ[T[b]]]

Print["ValueQ(Legacy)", ValueQ[T[a], Method->"Legacy"], ValueQ[T[b], Method->"Legacy"]]

Print["MyValueQ", MyValueQ[T[a]], MyValueQ[T[b]]]

gives:

In[1]:= SetAttributes[MyValueQ, HoldAll]

In[2]:= MyValueQ[x_] := Not[x === Unevaluated[x]]

In[3]:= T[a] = 1

Out[3]= 1

In[4]:= Print["ValueQ", ValueQ[T[a]], ValueQ[T[b]]]
ValueQTrueTrue

In[5]:= Print["ValueQ(Legacy)", ValueQ[T[a], Method->"Legacy"], ValueQ[T[b], Method->"Legacy"]]
ValueQ(Legacy)TrueFalse

In[6]:= Print["MyValueQ", MyValueQ[T[a]], MyValueQ[T[b]]]
MyValueQTrueFalse

ie MyValueQ reproduces the results of the old ValueQ.

The issue with 12.2 is that ValueQ returns True if any symbol in its argument is defined and in Kranc's case in the query ValueQ[TensorCharacters[T]] that T has a definition present.

barrywardell commented 3 years ago

You can also work with Ian's original solution but insert and Evaluate in the right place:

ValueQ[A, Evaluate[If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]]]
rhaas80 commented 3 years ago

You can also work with Ian's original solution but insert and Evaluate in the right place:

ValueQ[A, Evaluate[If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]]]

I am not sure about that. If I run that snipped on 12.0 I get:

$ wolfram
Mathematica 12.0.0 Kernel for Linux x86 (64-bit)
Copyright 1988-2019 Wolfram Research, Inc.

In[1]:= ValueQ[A, Evaluate[If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]]]

Out[1]= ValueQ[A, Null]

and

If[ValueQ[A, Evaluate[If[$VersionNumber >= 12.2, Method -> "Legacy", Sequence[]]]], Print["True"], Print["False"]]

returns the If unevaluated. My guess would be that one needs to do a similar trick like you are doing in McLachlan_BSSN.m to swallow the comma.

rhaas80 commented 3 years ago

The next ET release is scheduled to happen in ~3 weeks (end of May). It would of be good to have a fix included in Kranc for it. Is there a chance of the bugfix in https://github.com/ianhinder/Kranc/issues/151#issuecomment-785897792 be reviewed and incorporated into Kranc master by then?

barrywardell commented 3 years ago

It seems like a reasonable fix to me. I think the only questions are:

  1. Does your valueQ always behave exactly like the old ValueQ?
  2. ValueQ appears several times throughout Kranc. Should it be replaced everywhere, or only the one problematic location?
rhaas80 commented 3 years ago

Ah, had some offline discussion with Steve who suggested that maybe this:

SetAttributes[MyValueQ, HoldAll]
MyValueQ[x_] := If[$VersionNumber >= 12.2, ValueQ[x, Method -> "Legacy"], ValueQ[x]]

since that will put the burden of "works as before" on MMA's "Legacy" method.

rhaas80 commented 3 years ago

I thought about changing it everywhere just to be safe. When I looked into that i could not immediately find a good place for where to define MyValueQ so that it is defined in only a single file yet accessible everywhere. Is there file that all Kranc source files include? Somehting like "KrancDefinitions.m"?

ianhinder commented 3 years ago

Hi, I was also looking into this issue this morning, but then was AFK for the afternoon so missed all the above!

I am in the process of upgrading Mathematica to 12.2.0 for the purpose of implementing and testing this for McLachlan, on both 12.0.0 and 12.2.0, but I'm totally overloaded at the moment, so if there is someone who could do this instead, that would be great! I'm happy to review a PR.

rhaas80 commented 2 years ago

here's a pull request: https://github.com/ianhinder/Kranc/pull/152

Tested that with the pull request and MMA 12.2.3 one can generate McLachlan master.

The pull request introduces ValueQLegacy in Kranc.m and uses it everywhere except RunKranc.m.