Redesigning UnitalChunk constructors

[samm82]

There has been discussion throughout a few issues that seems to hinge on the constructors for UnitalChunks; since they were so interconnected, I decided to list the issues I've noticed with the constructors and propose a new design that would better organize them. Please let me know what's good, what's bad, and what's missing!

Note that I didn't address ucuc or ucw for this issue, since they are quite different and this issue is long enough.

The Problems

1. Assuming the Space is Real

While a UnitalChunk is simply a DefinedQuantityDict with a UnitDefn, most UnitalChunk constructors create the DefinedQuantityDict internally instead of having it passed as a parameter. Of these, most of them assume the Space to be Real.

Drasil/code/drasil-lang/lib/Language/Drasil/Chunk/Unital.hs

Lines 30 to 32 in 09365d7

	data UnitalChunk = UC { _defq' :: DefinedQuantityDict
	, _uni :: UnitDefn
	}

While (as far as I can tell) most quantities have a Real Space, I'm not sure if this should be assumed by the constructors for two main reasons.

1. It essentially doubles the number of required constructors: for each variant UnitalChunk constructor, it is likely that we will eventually need a variant that does NOT assume that the Space is Real. (This is actually the crux of What to do about dqd' constructors that result in chunks that are instances of HasSpace? #3075, although I didn't quite understand what was actually happening.)
2. It is not consistently used: there are many instances where a constructor that assumes the Space is Real could be used, but isn't. For example, the UnitalChunk constructor ucs' does not assume the Space to be Real, yet most of its calls use Real (all in Data.Drasil.Quantities.Math, 33/38 times in the unitals for GamePhysics, etc.) This is exacerbated when a UnitalChunk constructor that does not assume the Space is passed Real from a higher-level constructor.

@JacquesCarette also mentioned that assuming the Space to be Real is a hack in this comment, proposing we limit the scope of the drasil-lang constructors and have any "convenience" constructors be defined in drasil-utils to be "imported 'qualified' ... to indicate that ... assumptions are being used." For example, all quantities in Data.Drasil.Quantities.Math use the Real Space. I'm not sure if this should be used for cases like SWHS (which has exactly one ConstrConcept that does not have a Space of Real, shown below), but that seems like a different discussion.

Drasil/code/drasil-example/swhs/lib/Drasil/SWHS/Unitals.hs

Lines 414 to 419 in 09365d7

	-- Constraint 18
	tempW = cuc' "tempW"
	(nounPhraseSP "temperature of the water")
	"the average kinetic energy of the particles within the water"
	(sub (eqSymb temp) lWater) centigrade (Vect Real)
	[physc $ Bounded (Inc, sy tempInit) (Inc, sy tempC)] (exactDbl 0)

2. Building from a Concept c or its parts

We have two constructors that build a UnitalChunk from a Concept c...

Drasil/code/drasil-lang/lib/Language/Drasil/Chunk/Unital.hs

Lines 64 to 67 in 09365d7

	-- | Used to create a 'UnitalChunk' from a 'Concept', 'Symbol', and 'Unit'.
	-- Assumes the 'Space' is Real.
	uc :: (Concept c, IsUnit u) => c -> Symbol -> u -> UnitalChunk
	uc a b = ucs' a b Real

and five that build from "a given 'UID', term, and definition".

Drasil/code/drasil-lang/lib/Language/Drasil/Chunk/Unital.hs

Lines 74 to 78 in 09365d7

	-- | Similar to 'uc', except it builds the 'Concept' portion of the 'UnitalChunk'
	-- from a given 'UID', term, and definition (which are the first three arguments).
	uc' :: (IsUnit u) => String -> NP -> String -> Symbol -> u -> UnitalChunk
	uc' i t d s u = UC (dqd (dcc i t d) s Real un) un
	where un = unitWrapper u

Since we have a similar number of instances of both approaches (by my count, 137 where the Concept is passed vs. 150 where the Concept is built), I'm not sure if we should remove the constructor(s) that build(s) the Concept. It might be good to force the user to create the Concept themselves to ensure it is done correctly; this would also allow us to remove separate UnitalChunk constructors for taking a String or a Sentence for the description (shown below).

Drasil/code/drasil-lang/lib/Language/Drasil/Chunk/Unital.hs

Lines 86 to 96 in 09365d7

	-- | Similar to 'uc'', but does not assume the 'Space'.
	ucs :: (IsUnit u) => String -> NP ->
	String -> Symbol -> Space -> u -> UnitalChunk
	ucs nam trm desc sym space un = UC (dqd (dcc nam trm desc) sym space uu) uu
	where uu = unitWrapper un
	-- | Similar to 'ucs', but uses a 'Sentence' for description.
	ucsWS :: (IsUnit u) => String -> NP ->
	Sentence -> Symbol -> Space -> u -> UnitalChunk
	ucsWS nam trm desc sym space un = UC (dqd (dccWDS nam trm desc) sym space uu) uu
	where uu = unitWrapper un

Alternatively, we could also implement these extra constructors in drasil-utils. Since it is trivial to build a Sentence from a String, we can also just remove the String-based constructor.

This has some implications, like potentially making the code less readable, as discussed in the wiki page for the DefinedQuantityDict chunk investigation. Also, since ucs is used in cuc', we would need to tweak it as well.

Drasil/code/drasil-lang/lib/Language/Drasil/Chunk/Constrained.hs

Lines 125 to 130 in 09365d7

	-- | Creates a constrained unitary chunk from a 'UID', term ('NP'), description ('String'), 'Symbol', unit, 'Space', 'Constraint's, and an 'Expr'.
	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

3. Missing constructors for Symbols that depend on the Stage.

This is related to points 1 and 2 above, but we currently only have one UnitalChunk constructor for when the Symbol is dependent on the Stage. As shown below, this constructor is based on uc' and as such, assumes the Space is Real (point 1) and builds the Concept from the passed parameters (point 2).

Drasil/code/drasil-lang/lib/Language/Drasil/Chunk/Unital.hs

Lines 80 to 84 in 09365d7

	-- | Similar to 'uc'', but 'Symbol' is dependent on the 'Stage'.
	ucStaged :: (IsUnit u) => String -> NP -> String -> (Stage -> Symbol) -> u ->
	UnitalChunk
	ucStaged i t d s u = UC (dqd' (dcc i t d) s Real (Just un)) un
	where un = unitWrapper u

This is the core problem in #3074, where again, I didn't really understand what was happening. I was seeing if there is was a way to split a Concept into its parts to be passed into this constructor; there was a bit of an afterthought of creating another constructor that took a Concept, but I didn't look into that too much.

4. Poor Naming

This is a common thread for all constructors. uc and ucs' seem to be paired up (both have Concepts being passed where ucs' also takes a Space), as do uc' and ucs (both have Concepts being built where ucs also takes a Space), which seems inconsistent to me. makeUCWDS is also the only constructor to not start with "uc", which is also inconsistent. ucStaged is also based on uc', not on uc as one would expect.

My Proposed Solution

The solution would address point 1 by not assuming the Space at all (although some helper functions that do could be created in drasil-utils if desired). The main two constructors would be uc and ucStaged. As per point 3, ucStaged is a variant to allow for Symbols to depend on the Stage, and is based on uc to resolve point 4.

-- | Used to create a 'UnitalChunk' from a 'Concept', 'Symbol', 'Space', and 'Unit'.
uc :: (Concept c, IsUnit u) => c -> Symbol -> Space -> u -> UnitalChunk
uc a sym space u = UC (dqd (cw a) sym space un) un
  where un = unitWrapper u

-- | Similar to 'uc', but 'Symbol' is dependent on the 'Stage'.
ucStaged :: (Concept c, IsUnit u) => c -> (Stage -> Symbol) -> Space -> u -> UnitalChunk
ucStaged a sym space u = UC (dqd' (cw a) sym space (Just un)) un
  where un = unitWrapper u

Depending on where we land on point 2, the following two constructors could also be included (either here or in drasil-utils). As mentioned above, these both only take a Sentence as the description (as opposed to a String), although this is also not set in stone. As before, neither assumes the Space to be Real, resolving point 1. ucNoConcStaged is a variant for a Symbol that depends on the Stage, as per point 3. Finally, they also have more descriptive names (a variant of uc that doesn't take a Concept -> "no Concept" -> NoConc) which resolves point 4.

-- | Similar to 'uc', except it builds the 'Concept' portion of the 'UnitalChunk'
-- from a given 'UID', term, and definition (which are the first three arguments).
ucNoConc :: (IsUnit u) => String -> NP -> Sentence -> Symbol -> Space -> u -> UnitalChunk
ucNoConc i t d sym space u = UC (dqd (dccWDS i t d) sym space un) un
  where un = unitWrapper u

-- | Similar to 'ucNoConc', but 'Symbol' is dependent on the 'Stage'.
ucNoConcStaged :: (IsUnit u) => String -> NP -> Sentence -> (Stage -> Symbol) -> Space -> u -> UnitalChunk
ucNoConcStaged i t d sym space u = UC (dqd' (dccWDS i t d) sym space (Just un)) un
  where un = unitWrapper u

[JacquesCarette]

Numbering things the same as above.

1. So having combinators that hardwire Real was my idea: so many of the chunks had this, it seemed like clutter. Thus I thought, let's have some specialized ones. The conclusion that I draw from this problem is that this was a (very) premature optimization that should be undone. We can revisit it later. In fact, many of the smart constructors for various chunks arose from the same idea of trying to be less noisy at the use site. That should likely be undone from drasil-lang. Utility smart constructors can indeed be re-introduced elsewhere.
2. We really do need to think a bit more about whether the intermediate Concept should exist separately or not. And if it does, what does it mean to build a Unital on top of it. This is a deep design decision, and needs some serious thinking. The String/Sentence issue indeed is much easier to deal with. I agree that removing the String-based constructors from drasil-lang makes sense.
3. Folliowing the above analysis, we really ought to have the most general version of a constructor that takes all the pieces and makes no assumptions at all.
4. Completely agree. First, we ought to reduce the number of constructors. Then we can come up with a decent naming convention based on what's left.

I don't like NoConc as part of the name. It's too implicit, i.e. you need to know what you're subtracting to know what it means.

Point 2 remains the thorniest. But UnitalChunk is as good a place as any (given the large number of them) to really study what's going on. Having all of them "at hand" (I know that's almost 300 chunks - so automation is key) for a proper analysis sounds like the next best step.

[samm82]

@JacquesCarette I found (as best as I could) every instance of where a UnitalChunk constructor is used. From drasil-lang, the ucs constructor is used in cuc', which is in turn used elsewhere.

-- Chunk.Constrained
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

-- Chunk.UncertainQuantity
uqc :: (IsUnit u) => String -> NP -> String -> Symbol -> u -> Space
                -> [ConstraintE] -> Expr -> Uncertainty -> UncertQ
uqc nam trm desc sym un space cs val = uq (cuc' nam trm desc sym un space cs val)

uqcND :: (IsUnit u) => String -> NP -> Symbol -> u -> Space -> [ConstraintE]
                  -> Expr -> Uncertainty -> UncertQ
uqcND nam trm sym un space cs val = uq (cuc' nam trm "" sym un space cs val)

This gives us the following break down:

From Concept

• uc: 84
• ucs': 69
• Total: 153

From Components

• uc': 41
• ucStaged: 4
• ucs: 11
• ucsWS: 16
• makeUCWDS: 44
• cuc': 5
• uqc: 24
• uqcND: 3
• Total: 148

There are some concepts that get reused for other defining concepts (for example, the area concept gets reused when defining the surArea concept for surface area) or in the actual document in instance models, etc. However, there are quite a few that are only created locally. As far as I can tell, most functions used to reference these concepts in the body of an example (see below for some examples) could be used on the underlying Concept instead of the UnitalChunk as a whole. Now we need to start unpacking the semantic difference between a Concept and a UnitalChunk, apart from the obvious "one has a unit" to know if we should always be storing the Concept separately.

Drasil/code/drasil-lang/lib/Language/Drasil/Development/Sentence.hs

Lines 48 to 54 in 1831438

	-- | Helper for getting the phrase from a 'NamedIdea' using it's UID.
	phrase :: (HasUID n, NamedIdea n) => n -> Sentence
	phrase n = sentenceTerm (n ^. uid)
	-- | Helper for getting the plural of a phrase from a 'NamedIdea'.
	plural :: (HasUID n, NamedIdea n) => n -> Sentence
	plural n = sentencePlural (n ^. uid)

I've logged the (hopefully) extensive list of uses of all UnitalChunk constructors on this wiki page for further investigation.

[smiths]

Your wiki page seems very thorough @samm82. This should help us improve our design. We can discuss further during our team meeting (#3202) tomorrow (Tuesday).

[samm82]

Next steps on this issue: redesign the UnitalChunk constructors following points 1, 3, and 4, and leave the discussion of point 2 to another date. (Some more things to think about in regards to point 2 might come up during the refactoring from the other points.)

@JacquesCarette what do you think about the names uc and ucFromConc or ucFromConcept to distinguish the creation of a UnitalChunk from the pieces of a Concept vs. from the Concept itself (respectively)?

[JacquesCarette]

Redesign UnitalChunk Constructors