Drasil: SWHS: Missing Info from Latent Heat TM Equation

[samm82]

The initial condition should be added:

[JacquesCarette]

I think the + is junk / misleading. You are quite right that Q(0)=0 is missing.

[smiths]

I agree with @JacquesCarette. I don't know why the plus is in the manual version, but we need the initial value (at t=0) for Q.

[samm82]

For a TM, you can enter a list of equations, which I did, but Q shows up twice in the Description, for its usage in both expressions. Is this OK? @JacquesCarette @smiths

[JacquesCarette]

These equations make no sense to me! They basically define Q as, well, Q. The equation above is true for any function Q that is continuously differentiable and 0 at 0. And, of course, the Wikipedia page doesn't do that, it defines things 'properly'.

To answer the question: no, it is no Ok for Q to appear twice in the description. That it does points to the self-referential nature of this definition too.

[samm82]

Should I add a constructor for Expr to build from an initial condition? ICond :: Expr -> Expr -> Expr so that ICond a b will produce "a, with b"? This will match the original equation.

[JacquesCarette]

No - the right fix is to add the concept of an ODE to 'models'. I had started that on my QDef branch.

[samm82]

Should I just leave this issue until the QDef branch gets merged in?

[JacquesCarette]

You (or maybe @danscime ) can have a real hand into taking the content of that branch and resurrecting it. The branch is dead - too stale to ever hope merging. The changes, as outlined in #1373 can be done bit-by-bit. The most important (for this discussion) being the introduction of ModelKind. And this should be used in both TM and IM. That would allow progress on this current issue.

[samm82]

While trying to merge in ModelKind, I'm getting the feared "Cannot find path from inputs to outputs" error:

cd build/GlassBR && stack exec -- glassbr
glassbr.EXE: Cannot find path from inputs to outputs: ["isSafePb","isSafeLR"] given Defs as 
["safetyProbability","safetyLoad"] and Knowns as ["plateLen","plateWidth","standOffDist",
"char Weight","aspectRatio","pbTol","tNT","glassTypeCon","nomThick","sdx","sdy","sdz","minThick",
"loadDurFactor","gTF","eqTNTWeight","sflawParamM","sflawParamK","modElas","loadDur","lShareFac",
"dimMax","dimMin","arMax","cWeightMax","cWeightMin","sdMax","sdMin","sdfTol","demand","dimlessLoad",
"tolLoad","stressDistFac","nonFactorL","riskFun","lRe","probBr"]
CallStack (from HasCallStack):
  error, called at .\Language\Drasil\CodeSpec.hs:331:20 in drasil-code-0.1.7-FW4WiLnxBpJ46FU06JCbfZ:Language.Drasil.CodeSpec
make: *** [glassbr_gen] Error 1

I've looked through previous issues, but am unsure as to resolve it, as the one commit that might have solved it in the past is "unavailable due to large server load". I'll push my changes in case you want to take a look @JacquesCarette.

---

Also, with the most recent change in #1479 of making the ODE a case of the equation, using ModelKind might be a little more difficult, but I haven't looked at everything in the QDef branch in detail yet so I'm not 100% sure.

[bmaclach]

@samm82 Just as some background information, because I'm not sure how familiar you are with that kind of error, it gets thrown by the code generator when it cannot find a sequence of equations that can be used to get from the inputs to the outputs. Did you change the inputs list, outputs list, or equations?

[bmaclach]

@samm82 I took a quick look at your code, and I suspect the problem is that all of the IM constructors create an IM with ModelKind of OthModel, but the getEqMod function ignores all ModelKinds other than EquationalModel.

[samm82]

Ohhh ok - I'll start merging in the next commits that fix that. 👍 Thanks @bmaclach!

[samm82]

@JacquesCarette The current constructors for IMs include im, imNoDeriv, imNoRefs, and imNoDerivNoRefs. Should I create a variant of each of these for each IM type (which would result in 12 IM constructors), or should I make them also take in a type of IM to build (EquationalModel, DEModel, or OthModel)?

[samm82]

@bmaclach I tried making an Equational IM constructor and got the same error again - would you be able to take a look?

[bmaclach]

@samm82 I gave it a look. So, in the error message, the part that says "with Defs as [...]" tells you which equations it's looking at for trying to find this path from inputs to outputs (the UIDs of the QDefinitions are what you see in the list). The only 2 UIDs it's showing are those of the TMs... so for some reason it's missing both the IMs and the DDs, it seems. Why that is, I don't know. I'll keep looking but wanted to catch you up with what I've found so far.

[bmaclach]

@samm82 My last comment was a little misleading. The "Defs" shown in the error message are actually the equations that haven't been used yet on the "path". After further investigation, it is recognizing DDs and IMs but those aren't showing up in the error message because it has already used them. The problem, then, is that it hasn't recognized that what was calculated by the IMs is the same as the outputs.

[JacquesCarette]

I definitely encountered that problem too. Part of it is exactly as @bmaclach says: you need to make sure you have things as EquationalModel. No, there shouldn't be 12 IM constructors! Right now, we might need 3 -- variants of im for each Kind. So 2 new ones, I guess im should probably be for either OthModel or EquationalModel (whichever needs fewest changes to the examples).

Then you'll just need to make sure everything that's needed is visible. Look at that branch (when you can) to see what I had to do.

[bmaclach]

@samm82 One other thing I noticed: There is a third output, probBr, which you'll notice is not included in the first list in the error message. That means Drasil succeeded in calculating that output, so you know that the DD that defines probBr works. Then you can compare the QDefinition used in that DD to the ones you are writing for the IMs and find any differences that might have broken things.

[samm82]

I implemented the QDs using mkQuantDef (like probBr does) and GlassBR now generates. It doesn't match stable (because of how I made it) but it least it generates. I'll look into it in more detail tomorrow.

[samm82]

I've narrowed down the current differences in output of GlassBR to two points and I need help with both of them:

---

1) For @bmaclach, although the values (isSafeLoad and such) are defined in the Boolean space, they functions for calculating them are being rendered as doubles:
eg.

< bool func_is_safeLR(double LR, double q) {
---
> double func_is_safeLR(double LR, double q) {

I think this might be an issue with code generation and I'm not sure how to resolve it.

---

2) For @smiths, the labels for the IMs now use the description for their corresponding QuantityDict unitals. I'm unsure about the work you were doing with Vajiheh - is this a desired change? My guess is not.
eg.

< Label & Safety Req-LR
---
> Label & Variable that is assigned true when load resistance (capacity) is greater than load (demand)

[smiths]

@samm82, you are correct that this is not a desired change. The label should be short, not a full description. Hopefully you can revert it back to the way this was before the changes by Vajiheh.

[samm82]

@smiths Again, just to clarify, the TMs and IMs now are very similar, and the old IM (calOfDemand) got removed. Is this correct?

[smiths]

@samm82, can you please include screenshots of what is similar and what is removed?

[bmaclach]

@samm82 I looked into the first difference (boolean functions being rendered as double) and discovered that our constructors for QDefinition (fromEqn, fromEqn', fromEqnSt, and fromEqnSt') all overwrite the Space to Real. They are all marked with a "FIXME: Space hack", so this is definitely something we should try to fix. mkQuantDef uses these functions, so that's why we are seeing this now. Probably we want to update those 4 functions to take a Space as a parameter. It may not be so simple, because there was probably a reason that whoever first wrote them resorted to hard-coding the Space, but at least we can find the issue that is causing us to need that hack.

[samm82]

@smiths Never mind - it looked like a mistake, but I think GlassBR just underwent the same process as some of the other examples in terms of refinement and reorganization.

Do we still want the definitions like "variable that is assigned true when load resistance (capacity) is greater than applied load (demand)"? Currently, the label comes from the definition of what concept is defined by the IM. I'm not sure what the intent of these definitions was, but I think replacing them with "Safety Load", etc. would be desired, even if this change propagates to the Table of Symbols. The current definition gives too much information about how it is obtained and not enough about what it represents in my opinion.

[JacquesCarette]

Regarding the Space hack: this is an issue that arose out of being over-zealous with respect to convenience. As most of the cases involve Real values, it was a pain to always pass that in, it seemed redundant. Except when it's not, of course!

What we really want is for drasil-lang to export a much smaller set of constructors that are inconvenient to use, but precise. And then drasil-utils can have sets of additional constructors that build in some assumptions; I think the convention should be that the latter should be imported 'qualified' (and thus only exported by a sub-module of drasil-utils rather than by the top-level) to indicate that these assumptions are being used.

That would also allow us to clean up the mess in drasil-lang!

[samm82]

@JacquesCarette I _really_ like that idea!

[smiths]

From @samm82

Do we still want the definitions like "variable that is assigned true when load resistance (capacity)
is greater than applied load (demand)"? Currently, the label comes from the definition of what
concept is defined by the IM. I'm not sure what the intent of these definitions was, but I think
replacing them with "Safety Load", etc. would be desired, even if this change propagates to the
Table of Symbols. The
current definition gives too much information about how it is obtained and not enough about what
it represents in my opinion.

I agree that "variable that is assigned true etc." is not an abstract definition. It is also redundant to say it is assigned true. I would like to change the definitions along the lines of what you are proposing. Could you list the IMs along with your newly proposed definitions. We can have a quick conversation and then you can port this change to Drasil.

[samm82]

@smiths Before, the idea that they were "safety requirements" was used; how about "Probability of Breakage Safety Requirement" and "Load Resistance Safety Requirement"?

The following unitals would be involved:
https://github.com/JacquesCarette/Drasil/blob/21495fc575b6fd00a9b479c5b876ef4b97352a9e/code/drasil-example/Drasil/GlassBR/Unitals.hs#L254-L268

I also think that the definition of these terms is duplicated and redundant (since they capture the exact same information - related to the questions I had before for Vajiheh). Are you sure they both should be kept, or should isSafeProb and isSafeLoad be removed and replaced with isSafePb and isSafeLR (albeit with the improved definitions)?

[samm82]

@bmaclach @JacquesCarette I fixed the Space hack in fromEqnSt and fromEqnSt' quite easily, and made a separate issue (#1580) for finishing the job.

[samm82]

@JacquesCarette However, as I mentioned earlier, the IM is now an EquationalModel instead of a DEModel because of the cases, so I'm not sure how I would add the initial condition.

[smiths]

Responding to:

https://github.com/JacquesCarette/Drasil/issues/1478#issuecomment-503242736

I like what you suggested for the terms, but I would make a small modification to change breakage to failure: "Probability of Failure Safety Requirement" and "Load Resistance Safety Requirement". Failure is a more general concept than breakage.

[samm82]

@smiths Should there still be two separate terms, or can each pair be merged?

[smiths]

Two separate terms. They are similar, but separate concepts.

[samm82]

Renamed safety requirements in a9cbe5665 @smiths

[samm82]

Should the variable used for the IM be called "Probability of Breakage Safety Requirement", since it is being refined? @smiths

[smiths]

@samm82, I thought we were previously talking about the TMs. If it is the IM, then yes, you should say probability of breakage, not failure.

[samm82]

Sorry, I was talking about both and was talking about merging the unitals, not the TMs and IMs 😬. Should the other IM still be "Load Resistance Safety Requirement"?

[smiths]

@samm82, can you please summarize all the moving parts so that I can see what you are getting at. If I have to open the document and go through everything, it will take me more time to answer your question. I'm not even sure that the question is anymore. 😄

[samm82]

The following unitals are involved:

https://github.com/JacquesCarette/Drasil/blob/a9cbe5665c4f956e2c735fe7a97758cb30d78fa9/code/drasil-example/Drasil/GlassBR/Unitals.hs#L254-L257

where the first of each pair is used in the IMs:

and the second in the TMs:

The IM unital isSafePb should be renamed to "Probability of Breakage Safety Requirement", and I think the other IM unital isSafeLR can stay the same. @smiths

[smiths]

This summary is great @samm82. Everything is one place, so our conversation can be quicker and more structured. Presented this way, I also see some things that I hadn't noticed before. :smile:

I agree with your recommendation. The IM isSafePb should be renamed "Probability of Breakage Safety Requirement" and isSafeLR can stay as it is.

The "new" things I noticed are as follows:

1. Can we constrain the probabilities to be between 0 and 1 (inclusive)? We discussed this before, but we only have a constraint for the probabilities being greater than 0.

2. I think I finally understand your questions about the TMs versus the IMs. We should have TMs, but their description is wrong. Their description isn't abstract at all; it is about glass. I apologize for not catching this when Vajiheh revised the SRS for GlassBR.

@samm82, can you please create issues to fix the above two problems. For the second one, the new text for the description should use the word structure in place of glass. We should also remove the connection between the two TMs. The nature of GlassBR means that they are both true or both false. We can't say this in general. Load is also not the 3 second duration. Load is a much more general concept here.

Vajiheh wasn't able to do this, but what I would really like is if we can use the same TM for GlassBR that we use for SSP. We are talking about load and capacity, in an abstract sense, in both cases. You don't actually have to worry about actually using the TM in both documents, but please revise the GlassBR text to make this an option in the future. You should discuss this with @bmaclach.

[JacquesCarette]

Responding to an old comment: if the model is a differential equation, then it should be a DEModel. If DEModel is not adequate for the job, it should be fixed. For example, DEModel should definitely know about initial conditions (it doesn't right now), and that's just a plain bug.

[samm82]

The model isn't a DE (anymore) - it's an equation with a DE for one of the cases. What if I was to make a DE constructor for Expr? @JacquesCarette

[JacquesCarette]

Now that I look at the top: this never was a DE. And I still think that the equation at the very top of this issue is bogus (i.e. mathematically meaningless). @smiths ?

[smiths]

@samm82, you are mixing multiple issues and discussions in one issue again. Please create new issues for new discussions. Why are we discussing GlassBR safety requirements under SWHS and latent heat.

We also discussed latent heat in issue #1479. It took me a while to find that discussion. If we are discussing latent heat energy, the full discussion should be under one heading.

I might have abused notation in someway, but I don't consider the equation mathematically meaningless. The additional information from #1479 is necessary, but the equation has to capture the addition of heat energy. We know how much heat energy is added because we know the rate of heat energy addition and the amount of time. If there is another way to express this, I'm very open to suggestions. :smile:

[samm82]

Migrated to #1593