Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Awurama-N opened this issue Jul 20, 2021 · 21 comments
Open

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

Awurama-N opened this issue Jul 20, 2021 · 21 comments
Assignees
Labels
design Related to the current design of Drasil (not artifacts). needs-clarification Needs a clear 'state', 'goal', 'analysis', and 'explanation' to reduce solution ambiguities.

Comments

@Awurama-N
Copy link
Contributor

Awurama-N commented Jul 20, 2021

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

@Awurama-N Awurama-N self-assigned this Jul 20, 2021
@Awurama-N Awurama-N changed the title Finding all calls for dqd, dqdNoUnit, dqd', dqdWr and dqdQd Finding all calls to dqd, dqdNoUnit, dqd', dqdWr and dqdQd Jul 20, 2021
@Awurama-N
Copy link
Contributor Author

Awurama-N commented Jul 20, 2021

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 constructors ucs’, 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
Copy link
Owner

  1. the dqdNoUnit can stay as is
  2. 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
Copy link
Contributor Author

Awurama-N commented Jul 21, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

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

@JacquesCarette
Copy link
Owner

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
Copy link
Contributor Author

Awurama-N commented Jul 22, 2021

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

smiths commented Jul 26, 2021

@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
Copy link
Owner

JacquesCarette commented Jul 27, 2021

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
Copy link
Owner

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 samm82 self-assigned this Sep 19, 2022
@samm82
Copy link
Collaborator

samm82 commented Oct 4, 2022

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
Copy link
Collaborator

peter-michalski commented Oct 5, 2022

For more context, the origin of this issue is related to #2677

@smiths
Copy link
Collaborator

smiths commented Oct 5, 2022

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
Copy link
Owner

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.

samm82 added a commit that referenced this issue Jan 12, 2023
I also verified that this works as intended using an example in Projectile as per #2704.
Related issue: #3074
@balacij
Copy link
Collaborator

balacij commented Apr 27, 2023

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?

@balacij balacij added design Related to the current design of Drasil (not artifacts). needs-clarification Needs a clear 'state', 'goal', 'analysis', and 'explanation' to reduce solution ambiguities. labels Apr 27, 2023
@samm82
Copy link
Collaborator

samm82 commented Apr 27, 2023

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
Copy link
Collaborator

smiths commented Apr 29, 2023

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
Copy link
Collaborator

balacij commented May 1, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Related to the current design of Drasil (not artifacts). needs-clarification Needs a clear 'state', 'goal', 'analysis', and 'explanation' to reduce solution ambiguities.
Projects
None yet
Development

No branches or pull requests

6 participants