Theory as an instance of the ExprRelat class?

[niazim3]

As I'm currently working through the errors that the shift from using Contents to LabelledContent is causing in the addLabels branch, I ran into the following error:

Drasil\GlassBR\Body.hs:172:24:
    • No instance for (ExprRelat TheoryModel)
        arising from a use of ‘relToQD’
    • In the first argument of ‘map’, namely ‘(relToQD gbSymbMap)’
      In the first argument of ‘(++)’, namely
        ‘(map (relToQD gbSymbMap) gbrTMods)’
      In the second argument of ‘(++)’, namely
        ‘(map (relToQD gbSymbMap) gbrTMods) ++ [wtntWithEqn, sdWithEqn]’

, where the code causing said error can be viewed at commit dab1afa and relToQD is defined as follows:

Drasil/code/drasil-code/Language/Drasil/CodeSpec.hs

Lines 149 to 155 in d8e343f

	-- medium hacks ---
	relToQD :: (ExprRelat c, HasShortName c, HasSymbolTable ctx) => ctx -> c -> QDefinition
	relToQD sm r = convertRel sm (r ^. relat) (shortname r)
	convertRel :: (HasSymbolTable ctx) => ctx -> Expr -> ShortName -> QDefinition
	convertRel sm (BinaryOp Eq (C x) r) sn = ec (symbLookup x (sm ^. symbolTable)) r sn
	convertRel _ _ _ = error "Conversion failed"

. Since TheoryModels are not instances of the class ExprRelat (please see Theory.hs here), the error above is being produced.

---

@szymczdm I had asked if TheoryChunk should be reworked to be built off of a RelationConcept like InstanceModel is in issue #695, and @JacquesCarette mentioned that it is good as is it is.
Do you have any suggestions on how to approach this issue?

I feel that I should also point out that the current implementation of TheoryModel shows quite a bit of duplication (see below from GlassBR/TMods.hs).

t1IsSafe :: TheoryModel
t1IsSafe = tm' (cw t1SafetyReq) 
  (tc' "isSafe" [qw is_safe1, qw prob_br, qw pb_tol] ([] :: [ConceptChunk])
  [] [TCon Invariant $ (sy is_safe1) $= (sy prob_br) $< (sy pb_tol)] [])
  (mkLabelRA'' "isSafeLabel") [t1descr]

t1SafetyReq :: RelationConcept
t1SafetyReq = makeRC "t1SafetyReq" (nounPhraseSP "Safety Requirement-1")
  t1descr ((sy is_safe1) $= (sy prob_br) $< (sy pb_tol)) Nothing--label

---

This issue is related to #628 and #387.

[szymczdm]

The constructor for TheoryModel could likely be updated, but I think the real problem here is ExprRelat. It is a pretty big hack that should be removed eventually.

I need to figure out exactly why we need relToQD. It looks like it just wants a defining equation for a chunk, and to convert that to a QDefinition and that seems weird (other than Theories defining a number of QDefs). I'll need to look at this in more depth.

[niazim3]

As of commit d788d4b, all examples (except GlassBR) are compiling with the use of LabelledContents and Labels. Since this was the case, I'm going to continue working on #695 (i.e. adding labels) with the Makefile on my machine removing all instances/calls for GlassBR.

Since adding labels was fairly high priority, I guess this issue also becomes somewhat of a high priority since the labels implementation cannot be merged into master until all examples are at least functioning.

---

Just to reiterate:
The original implementation of

Drasil/code/drasil-example/Drasil/GlassBR/Body.hs

Line 172 in 46e5161

(map (relToQD gbSymbMap) tModels {-[RelationConcept]-}) ++

causes the following error

Drasil\GlassBR\Body.hs:172:24:
• No instance for (HasLabel RelationConcept)
    arising from a use of ‘relToQD’
• In the first argument of ‘map’, namely ‘(relToQD gbSymbMap)’
  In the first argument of ‘(++)’, namely
    ‘(map (relToQD gbSymbMap) tmodels)’
  In the second argument of ‘(++)’, namely
    ‘(map (relToQD gbSymbMap) tmodels) ++ [wtntWithEqn, sdWithEqn]’

because RelationConcept has a Maybe Label instead of a Label

Drasil/code/drasil-lang/Language/Drasil/Chunk/Relation.hs

Lines 19 to 23 in d788d4b

	data RelationConcept = RC { _conc :: ConceptChunk
	, _rel :: Relation
	, _lbl :: Maybe Label
	}
	makeLenses ''RelationConcept

(see #695 (comment) for rationale).

On the other hand, attempting to replace the tmodels with a list of actual [TheoryModel] results in the error from the above comment.

And finally, removing the line causing issues altogether causes the following:

...
mkdir -p build/GlassBR
cd build/GlassBR && stack exec glassbr
glassbr.EXE: Cannot find path from inputs to outputs
CallStack (from HasCallStack):
  error, called at .\Language\Drasil\CodeSpec.hs:319:20 in drasil-code-0.1.3-Ark8BNkxRlr5tP8V1ppwu:Language.Drasil.CodeSpec
make: *** [glassbr_build] Error 1

.

[szymczdm]

Alright, I've taken a closer look and it's definitely the use of relToQD for code generation that is causing problems.

I think this is part of a larger problem that the code generation is based on an older version of Drasil than we are currently working with. It makes explicit use of the equat (QDefn only) and relat lenses.

We need to understand more of what exactly the code-gen side is trying to do with these definitions before we can solve the problem.

[JacquesCarette]

I've added my name to this as well. Can't promise I'll be able to look at it on Monday/Tuesday, but it is rather important to figure this out. I've played around with this before, and it is a little tricky. Now is as good a time as any to figure out how to unravel the problems here.

[niazim3]

With the referencing infrastructure almost complete in the addLabels branch, this issue is still high priority. @szymczdm @JacquesCarette

[niazim3]

Just noting here that the field that's causing errors at the moment (_definitions) eventually will be removed (as evident in the following FIXME comment:

Drasil/code/drasil-lang/Language/Drasil/SystemInformation.hs

Lines 36 to 37 in 99c6635

	, _definitions :: [QDefinition] --FIXME: will be removed upon migration to use of [DataDefinition] below
	, _datadefs :: [DataDefinition]

).

[JacquesCarette]

Added to my list of things to do today.

[JacquesCarette]

ExprRelat

[JacquesCarette]

This certainly does not fix the big problem in this issue, but it does fix the symptom. Now, the fix may not be fully adequate [this doesn't pull all the definitions from the theory model], but it has the right 'flavour'. One problem seems to be that commenting out the one line of code (that gave rise to this issue) doesn't seem to actually change anything in the output of GlassBR. So I can't quite tell if this 'fix' is sufficient to fully unblock.

The issue should not be closed quite yet. I am not sure that #628 captures everything that actually needs fixed.

[niazim3]

I actually still get the error message from CodeSpec.hs

mkdir -p build/GlassBR
cd build/GlassBR && stack exec glassbr
glassbr.EXE: Cannot find path from inputs to outputs
CallStack (from HasCallStack):
  error, called at .\Language\Drasil\CodeSpec.hs:319:20 in drasil-code-0.1.3-Ark8BNkxRlr5tP8V1ppwu:Language.Drasil.CodeSpec
make: *** [glassbr_build] Error 1

.
Can this perhaps be fixed with removing the definitions_ field from SystemInformation (and anywhere else QDefinitions are used in CodeSpec.hs) as mentioned in the comment below?

Drasil/code/drasil-lang/Language/Drasil/SystemInformation.hs

Lines 36 to 37 in 99c6635

	, _definitions :: [QDefinition] --FIXME: will be removed upon migration to use of [DataDefinition] below
	, _datadefs :: [DataDefinition]

)

---

I've been looking into this error for a while now:

error is defined in getExecOrder

Drasil/code/drasil-code/Language/Drasil/CodeSpec.hs

Lines 310 to 320 in 0b501b0

	getExecOrder :: HasSymbolTable ctx => [Def] -> [Known] -> [Need] -> ctx -> [Def]
	getExecOrder d k' n' sm = getExecOrder' [] d k' (n' \\ k')
	where getExecOrder' ord _ _ [] = ord
	getExecOrder' ord defs' k n =
	let new = filter ((`subsetOf` k) . flip (codevars') sm . codeEquat) defs'
	cnew = map codevar new
	kNew = k ++ cnew
	nNew = n \\ cnew
	in if null new
	then error "Cannot find path from inputs to outputs"
	else getExecOrder' (ord ++ new) (defs' \\ new) kNew nNew

which is called in CodeSpec here

Drasil/code/drasil-code/Language/Drasil/CodeSpec.hs

Line 95 in 0b501b0

execOrder = getExecOrder rels (allInputs ++ map codevar const') outs' db,

where outs' for GlassBR is

and db for GlassBR is

and rels is a combined list of GlassBR's QDefinitions and DataDefinitions.

@JacquesCarette @szymczdm Is there a method you suggest to fixing this error?

Edit: Is this error better as a new issue?

[JacquesCarette]

I think this error is a different issue. It is related to the current issue, but the fix will be different. I will look at this problem right now.

[JacquesCarette]

You are correct that the right fix is to migrate to using datadefs instead of definitions. However, this used to work, so there might be another bug. I'm investigating that.

[JacquesCarette]

Ok, some progress. By making the error message more informative, I now get

MacBook-Air-2:GlassBR carette$ stack exec glassbr
glassbr: Cannot find path from inputs to outputs: ["is_safePb","is_safeLR"] 
  given Defs as [] and 
  Knowns as ["plate_len","plate_width","standOffDist","char_weight","aspect_ratio","pb_tol",
    "tNT","glass_type","nom_thick","sdx","sdy","sdz","min_thick","loadDurFactor",
    "gTF","sflawParamM","sflawParamK","mod_elas","load_dur","loadDurFactor",
    "lShareFac","dim_max","dim_min","ar_max","cWeightMax","cWeightMin",
    "sd_max","sd_min","eqTNTWeight","sdf_tol","standOffDist","demand","tolLoad",
    "nFL","dimlessLoad","lResistance","stressDistFac","risk_fun","prob_br"]
CallStack (from HasCallStack):
  error, called at ./Language/Drasil/CodeSpec.hs:319:20 in drasil-code-0.1.3-GeRzK6TAC038pgEk2bVLJK:Language.Drasil.CodeSpec

So the problem is that the things we want to output have no known definitions! I think that this is because these used to be included in the definitions but now no longer are.

[JacquesCarette]

@niazim3 question: does code generation for glassbr work on the master branch? If so, that would definitely help me debug what's going on here. Or if not on current master, when do I have to rewind to?

[szymczdm]

@JacquesCarette master is still generating code for GlassBR as far as I can tell.

[JacquesCarette]

@szymczdm thanks. It is actually quite tricky how these have diverged. I've checked in more stuff in the addLabels branch to move this forward.

It basically used to work because things were built 'by hand' as it were. The theory models, as built, actually forget information (such as the definitions of exactly the 2 variables needed in GlassBR). The 'invariant' thing was a red herring. [It's weird, but is not the source of the problem.]

[samm82]

@JacquesCarette @szymczdm 5461058 seems to have fixed it - I added the missing quantities to the inputs in SystemInformation. Is this a viable solution for now?

[JacquesCarette]

Unfortunately not. Both quantities should be computed, and are in fact part of the output. I am fairly sure that if you add them to the input, the generated code will not be what we want [or at least, it shouldn't be; it could still be ok, but that might be because of further hacks].

[samm82]

@JacquesCarette The error message on my machine is

glassbr.EXE: Cannot find path from inputs to outputs: ["is_safePb","is_safeLR","prob_br"] 
  given Defs as ["risk_fun","stressDistFac","dimlessLoad"] and 
  Knowns as ["plate_len", "plate_width", "standOffDist", "char_weight", "aspect_ratio", "pb_tol", "tNT", 
     "glass_type", "nom_thick", "sdx", "sdy", "sdz", "min_thick", "loadDurFactor", "gTF", "sflawParamM", 
     "sflawParamK", "mod_elas", "load_dur", "loadDurFactor" , "lShareFac", "dim_max", "dim_min", 
     "ar_max", "cWeightMax", "cWeightMin", "sd_max", "sd_min", "sdf_tol", "tolLoad", "nFL"]

I just noticed our errors were different; not sure what the significance of this is.

[JacquesCarette]

Something else must have changed to have made those 3 items (instead of the previous 2) 'output's. But that's fine.

The problem is that those 3 outputs should also be in Knowns, because they are defined by formulas. That is the real issue to fix, to make sure those 3 formulas end up in Known. They used to be, but no longer are. You can see how (in master branch) tModel is added to what will eventually be Known; and in the addLabels branch, those equations are no longer added. That is the real source of the bug, as far as I can tell.

[samm82]

Based on your comments, @JacquesCarette, I made a change in b85dbfb that I think fixed it. Can you take a look? It's a bit of a hack, but it could work for now.

[JacquesCarette]

Right, I'll comment on that issue.

[JacquesCarette]

More progress on this (in a branch to be merged). _definition is gone. relToQD is gone too. ExprRelat is still around, but not used in a crucial way. As soon as that branch is merged, this can be marked as fixed.

[samm82]

@JacquesCarette Has the branch been merged in? relToQD is still used in the TMods of GlassBR, so I think there's still some more work to do here.

Drasil/code/drasil-example/Drasil/GlassBR/TMods.hs

Lines 34 to 45 in 30b68b7

	lrIsSafe :: TheoryModel
	lrIsSafe = tm (cw lrIsSafeRC)
	[qw isSafeLR, qw lRe, qw demand] ([] :: [ConceptChunk])
	[relToQD locSymbMap lrIsSafeRC] [(sy isSafeLR) $= (sy lRe) $> (sy demand)] [] [makeCite astm2009]
	"isSafeLR" [lrIsSafeDesc]
	where locSymbMap = cdb (thisSymbols) ([] :: [IdeaDict]) glassBRsymb
	([] :: [UnitDefn]) Map.empty Map.empty [] [] [] [] []
	[] []
	lrIsSafeRC :: RelationConcept
	lrIsSafeRC = makeRC "safetyReqLR" (nounPhraseSP "Safety Req-LR")
	lrIsSafeDesc ( (sy isSafeLR) $= (sy lRe) $> (sy demand))

Drasil/code/drasil-example/Drasil/GlassBR/TMods.hs

Lines 57 to 68 in 30b68b7

	pbIsSafe :: TheoryModel
	pbIsSafe = tm (cw pbIsSafeRC)
	[qw isSafePb, qw probBr, qw pbTol] ([] :: [ConceptChunk])
	[relToQD locSymbMap pbIsSafeRC] [(sy isSafePb) $= (sy probBr) $< (sy pbTol)] [] [makeCite astm2009]
	"isSafePb" [pbIsSafeDesc]
	where locSymbMap = cdb (thisSymbols) ([] :: [IdeaDict]) glassBRsymb
	([] :: [UnitDefn]) Map.empty Map.empty [] [] [] [] []
	[] []
	pbIsSafeRC :: RelationConcept
	pbIsSafeRC = makeRC "safetyReqPb" (nounPhraseSP "Safety Req-Pb")
	pbIsSafeDesc ((sy isSafePb) $= (sy probBr) $< (sy pbTol))

[JacquesCarette]

It has not. I will have to re-do that work, as that branch is now very stale. I'm hoping to get to it soon.

[JacquesCarette]

I don't think there is anything specific to do to take care of this issue beyond what is already being done. In fact, a close reading of the issue makes me think that it is actually already fixed.

[balacij]

I agree. I don't think we would need to add an ExprRelat instance for TheoryModel -- the Theory class gives much of what we want from TheoryModel nicely as is. Additionally, relToQD is already removed, the _definitions list in SystemInformation is replaced with a list of InstanceModels (which we use to get QDs, and hopefully eventually get the DE solvable methods from), and the QDs for everything else (DataDefinitions) are generated nicely.

Are we missing anything else? Do we perhaps want to change the definition of EquationalModels from EquationalModel QDefinition into EquationalModel DataDefinition?

[JacquesCarette]

I think you're right. And EquationModel can stay as QDefinition until we get a compelling reason to change it.