Awurama-N commented 3 years ago

Searching Drasil for all calls to dqd, dqdNoUnit, dqd', dqdWr and dqdQd

Awurama-N commented 3 years ago

dqdNoUnit

in Data.Drasil.Quantitiies.Physics:

restitutionCoef

in Data.Drasil.Quatities.Math:

gradient
normalVect
uNormalVect
unitVect
unitVectj
petpVect
euclidNorm

in Data.Drasil.Quantities.SolidMechanics:

poissnsR

in Drasil.GlassBR.Unitals:

aspectRatio
glassTypeCon
gTF
loadSF

Drasil.PDController.Unitals:

ipPropGain
ipDerivGain
ipSetPt
opProcessVariable

dqd’

in Data.Drasil.Quatities.Math:

pi_
posInf
negInf

in Drasil.SSP.Unitals:

constF
fs -fsMin
cords
earthqkLoadFctr
normToShear
scalFunc
numbSlices
minFunction
mobShrC
shrResC
varblV
index

in Drasil.Projectile.Unitals:

launAngle

in Drasil.SWHS.Unitals:

eta
meltFrac
fracMin
consTol
aspectRatio
aspectRatioMin
aspectRatioMax

in Language.Drasil.Chunk.Constrained: -ConstrConcept constructor: cuc’’

in Language.Drasil.Chunk.Unital:

UnitalChunk constructor ucStaged

dqdQd

In Database.Drasil.ChunkDB.GetChunk:

combine
combine’

in Drasil.GlassBR.IMods:

symb

dqd

in Drasil.GlassBR.Unitals:

standOffDist

in Drasil.Projectile.Unitals:

flightDur
landPos
launspeed
offset
targPos

in Drasil.PDController.Unitals:

ipStepTime
ipSimTime

in Language.Drasil.Chunk.Constrained:

cuc’

in Language.Drasil.Chunk.Unital:

UnitalChunk constructorsucs’, uc’, ucs,ucsWS,makeUCWDS

dqdWR

in Drasil.GlassBR.IMods:

symb

in Drasil.SSP.Requirements:

inputDataTable
nputsToOutput

in Drasil.SSP.Unitals:

symbols
inputs

in Drasil.GamePhysics.Unitals:

defSymbols

in Drasil.NoPCM.Body:

pi_

in Drasil.SWHS.Unitals:

symbols
unitless

in Language.Drasil.Chunk.Constrained:

ConstrConcept constructors constrained’, constrainedNRV’, cnstrw’

All defined in Language.Drasil.Chunk.DefinedQuantity

@JacquesCarette

JacquesCarette commented 3 years ago

the dqdNoUnit can stay as is
the calls to dqd should be changed to calls to ucs' (from Unitals), except for the ones in Language.Drasil.Chunk.Unital

This change should be done "incrementally", in the sense that the ones that are very easy to change from dqd to ucs' should be just done (in one PR) and the ones that are not straightforward should be analyzed here.

Awurama-N commented 3 years ago

for changing dqd in the ConstrConcept constructors:

cuc' :: (IsUnit u) => String -> NP -> String -> Symbol -> u
            -> Space -> [ConstraintE] -> Expr -> ConstrConcept
cuc' nam trm desc sym un space cs rv =
  ConstrConcept (dqd (cw (ucs nam trm desc sym space un)) sym space uu) cs (Just rv)
  where uu = unitWrapper un

to

cuc' :: (IsUnit u) => String -> NP -> String -> Symbol -> u
            -> Space -> [ConstraintE] -> Expr -> ConstrConcept
cuc' nam trm desc sym un space cs rv =
  ConstrConcept (dqd (cw (ucs nam trm desc sym space un)) sym space uu) cs (Just rv)
  where uu = unitWrapper un

There's an error of " • Couldn't match expected type ‘DefinedQuantityDict’ with actual type ‘Language.Drasil.Chunk.Unital.UnitalChunk’ "

I see that dqd and ucs' essentially do the same / very similar things but, dqd creates a DefinedQuantityDict and ucs' creates a unitalChunk. & ConstrConcept has an accessor function of _defq :: DefinedQuantityDict

data ConstrConcept = ConstrConcept { _defq    :: DefinedQuantityDict
                                   , _constr' :: [ConstraintE]
                                   , _reasV'  :: Maybe Expr
                                   }
makeLenses ''ConstrConcept

is the right move to edit ConstrConcept or we don't want to touch that? alternatively would you like me to push the change instead so you see the error through github?

JacquesCarette commented 3 years ago

We're going to want to hold off on that, because it is going to be tricky. So, for now, leave it as dqd. Your comment was detailed enough that I didn't need more, thank you.

What we'll need to do will be to add a ConstrUnitalConcept that replaces DefinedQuantityDict with UnitalChunk. Let's deal with some of the other kinds of dqd constructors first, as that might influence the design.

Awurama-N commented 3 years ago

okay, then should i go ahead and replace dqd' with ucStaged

JacquesCarette commented 3 years ago

Yes - although it will not be a fully straightforward swap, because of Maybe. The ones with Just should go to ucStaged. Not sure about the Nothing ones -- maybe leave them as is?

Awurama-N commented 3 years ago

please for dqd & dqdWr what would you suggest the constructors to replace them be? I've tried switching them out but have come up unsuccessful that far.

I see dqdWr phrases should be in this structure:(Quantity c, Concept c, MayHaveUnit c) => c -> DefinedQuantityDict which is unlike any unital consructor currently in the file, should a new better suited one be made.

And lastly for scenario's like this:

symb :: [DefinedQuantityDict]
symb = map dqdWr [plateLen, plateWidth, charWeight, standOffDist] ++ 
  [dqdQd (qw calofDemand) demandq]

After switching out the constructors would you like the type to be changed to UnitalChunk ? or do we not want to touch that yet.

JacquesCarette commented 3 years ago

For dqdWr what it's doing is to take something that has off the information already but "maps it down". Probably the first thing to do is to create one for UnitalChunk that uses (Quantity c, Concept c, HasUnit c) as the constraints. But basically what you're going to have to do is to go through each call to dqdWr and see what type it's being applied to. dqdWr is basically a kind of 'cast'.

For things like symb: if it turns out that all of them have units, then yes, switch to UnitalChunk. If not, we'll have to think harder.

For dqd, that should be easy, since all of them have units. Pick the most appropriate one for that.

Awurama-N commented 3 years ago

for aspect ratio in GlassBr/ Unitals it says it has no unit & uses the constructor dqdNoUnit but shouldn't aspect ratio be metre over metre?

aspectRatio = uq (constrained' (dqdNoUnit aspectRatioCon (variable "AR") Real)
  [ physc $ UpFrom (Inc, exactDbl 1), 
    sfwrc $ UpTo (Inc, sy arMax)] (dbl 1.5)) defaultUncrt

I see it has been done before with radian here in Data.Drasil.Si_Units:

radian = derCUC' "radian" 
  "radian" "angle" (label "rad") (metre /: metre)

Should this be edited so aspectRatio could also be switched from using dqdNoUnit to something else?

Awurama-N commented 3 years ago

Also these 3 below are the only other instances that use unital chunk constructors but don't have units themselves. So should UnitalChunks always have units? instead of maybe having them? and then that could be edited as well, so Unital chunks always haver units.

ipPropGain
  = constrained' (dqdNoUnit propGain symKp Real) [gtZeroConstr] (exactDbl 20)
ipPropGainUnc = uq ipPropGain defaultUncrt
qdPropGain = qw ipPropGain

ipDerivGain
  = constrained' (dqdNoUnit derGain symKd Real) [physc $ UpFrom (Inc, exactDbl 0)]
      (exactDbl 1)
ipDerGainUnc = uq ipDerivGain defaultUncrt
qdDerivGain = qw ipDerivGain

ipSetPt = constrained' (dqdNoUnit setPoint symYrT Real) [gtZeroConstr] (exactDbl 1)
ipSetPtUnc = uq ipSetPt defaultUncrt
qdSetPointTD = qw ipSetPt

found in Drasil.PDController.Unitals

@smiths @JacquesCarette

smiths commented 3 years ago

@JacquesCarette is the expert on this topic. However, the units of m/m are a bit misleading. The units could be ft/ft, or 1000 m/1 km. The result is dimensionless. The unit is 1. However, saying the units are m/m does capture some information. If it was a dimensionless temperature, the units would be K/K (Kelvin over Kelvin). In both cases the units are 1, but keeping the ratio gives us some information that might be useful at some point.

JacquesCarette commented 3 years ago

Right - units cancel. So metre/metre is understood as 'unitless', because it's really metre * metre ^ (-1) = metre ^ 0 = 1.

So while metre /: metre is not wrong, it's also not quite what is expected. Our system doesn't expect these, so let's not do that right now.

JacquesCarette commented 3 years ago

Which brings us to a trick part: there is a difference between dimensionless and has no units !!! I mean, in 'physics', not merely in Drasil. A dimensionless quantity can arise as a computation involving quantities-with-units (when they all cancel); once you have units, you always have units.

So we ought to have a dimensionless constructor for UnitalChunk. It is a unit, but with every component being 0.

samm82 commented 1 year ago

okay, then should i go ahead and replace dqd' with ucStaged

An update on the status of the remaining dqd' calls in the code (ignoring those in Language.Drasil.Chunk.Unital):

Projectile Example

Using ucStaged would require passing in pieces of a prexisting ConceptChunk to be reconstructed behind the scenes. Is this desired?

If So...

ucStaged takes a String for its definition, but the definition of a ConceptChunk is stored as a Sentence. Is there a way to map from a Sentence to a String?

If Not...

should another helper be defined similarly to ucStaged that takes a Concept? (The issue from the If So... section will likely arise here as well.)

SSP/SWHS Examples

All calls to dqd' have no units. Do we pursue this or leave it for now?

Chunk/Constrained

The function is used in cuc'', but replacing it would create a ConstrConcept without a Space - presumably ConstrConcepts need Spaces as they are instances of HasSpace.

Chunk/Eq

Most calls to dqd' here have no units, like in the SSP/SWHS Examples. The two that don't, fromEqnSt and ec, have a similar issue to Chunk/Constrained. Replacing dqd' with ucStaged would create a QDefinition without a Space - presumably QDefinitions need Spaces as they are instances of HasSpace.

peter-michalski commented 1 year ago

For more context, the origin of this issue is related to https://github.com/JacquesCarette/Drasil/issues/2677

smiths commented 1 year ago

As we discussed in #3066, the next step is to create issues related to specific examples that seem off. The examples might be providing dummy values or use a Maybe.

JacquesCarette commented 1 year ago

Sentence -> String is always a "design smell". We never want to 'downgrade' a Sentence to a String. We might ask why ucStaged wants a String. That would be informative.

For dqd' with no units: is there another better combinator that could be used instead? Is it 'correct' that all of those don't have units?

Chunk/Constrained is indeed one of the places where things go seriously sideways. What kind of stuff doesn't have a Space? Seems weird. For all of these, we need the actual calls to "see" what's really going on.

balacij commented 1 year ago

I'm not quite sure what the ultimate goal of this ticket is (nor why/what the problem is). @samm82, seeing as you assigned yourself, do you think you can update the original post? Or add an extra explanation somewhere?

samm82 commented 1 year ago

I believe this issue was originally to remove the use of Maybes in constructors, but has since morphed into understanding and eventually redesigning how we build chunks

smiths commented 1 year ago

Hasn't the original purpose of this issue been satisfied? The issue's title says to find all calls to dqd, dqdNoUnit, dqd', dqdWr and dqdQd. I see a list of calls included above. I wonder if we should close this issue, but create new issues related to the needed improvements that have been identified. If the issue is a discussion of the redesign of how we build chunks, then we should link it to the other issues related to this task.

balacij commented 1 year ago

It has been, yes. If we could do that, I think it would be best. @samm82 do you know what tickets could be spun off?

JacquesCarette / Drasil

Finding all calls to dqd, dqdNoUnit, dqd', dqdWr and dqdQd #2704

dqdNoUnit

dqd’

dqdQd

dqd

dqdWR

Projectile Example

If So...

If Not...

SSP/SWHS Examples

Chunk/Constrained

Chunk/Eq