As pointed out in issue #514, "it appears ConVar and DefinedQuantityDict are identical in terms of the class instances they implement. The only difference is in the way they're constructed. If anything it might be worthwhile to remove one of them entirely since they're not really saying anything different."
Since ConVar only has ConceptChunk, Symbol, and Space and DQD has Quantity (which has unit, attribs, idea, space, and symbol) and ConceptChunk, ConVar is the type that should be removed.
Other notes for convenience:
"...we may want to change the way DQD is constructed to avoid the 'double uid' issue mentioned in the comments inside DefinedQuantity.hs.
Also, just to be clear, ConVar and DQD have exactly the same class instances. While ConVar always returns Nothing for the Maybe Unit, whereas DQD will sometimes have a non-Nothing value, they are databases of the same types of information.
Working on this issue in this branch.
niazim3
All 33 comments
[I've read all the conversation at #514]. I believe I agree.
However we should first figure out where ConVar is used/defined, and where a defined quantity is instead. There might be differences in intension that are not reflected in the implementations. If we prematurely unify them, we might regret it.
So I'd like to see say 5 different places each (with details!) of use "in context".
JacquesCarette
on 14 May 2018
It looks like DefinedQuantityDict instances are not that common in the current examples:
Example of use #1:
Looking in SSP/Unitals.hs:
https://github.com/JacquesCarette/Drasil/blob/dbd4b7f149c87c87794d7d9db21a1e0cd05d5972/code/Example/Drasil/SSP/Unitals.hs#L14-L16
, where sspUnitless is a list of convars.
If anything, I think the DefinedQuantityDict type is acting as a wrapper to create the list of symbols for SSP and SWHS/NoPCM. (Is this the type of example being looked for @JacquesCarette?)
niazim3
on 14 May 2018
Yes, this is exactly the kinds of examples I was looking for.
I would search for the constructors, not just the type itself. Because I think that there are specific slots in SystemInformation that expect a DefinedQuantityDict and/or ConVar.
JacquesCarette
on 14 May 2018
@JacquesCarette In SystemInformation:
https://github.com/JacquesCarette/Drasil/blob/51dbdb37f68961d4cad6f1212e46feb4b7b7c5ee/code/Language/Drasil/SystemInformation.hs#L23-L41
the only slots that could take either a DQD or a ConVar are b and f, which correspond to kind and concepts. A DQD could be accepted for e, h, and i, (quants, inputs, and outputs), but not Convar, since ConVar doesn't have Attributes.
samm82
on 14 May 2018
Thanks.
JacquesCarette
on 14 May 2018
@JacquesCarette Do you think unifying DQD and ConVar is a good idea at this point or no?
samm82
on 14 May 2018
It still needs further investigation. I need to look at what you've provided above in more detail.
JacquesCarette
on 14 May 2018
Should I find more examples of the usage of ConVar?
samm82
on 14 May 2018
All uses preferably.
JacquesCarette
on 14 May 2018
Language
ConVar is used to generate UnitalChunks, which could also be done via a DQD:
The only notable difference is that ConVar doesn't have Attributes, but these could be supplied by the DQD, unless there would be different Attributes for the UnitalChunk and the DQD it would be made from. All of the UnitalChunk's properties (uid, term, etc.) could be derived from a DQD.
The function cv is defined within ConVar.hs:
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Language/Drasil/Chunk/ConVar.hs#L35-L36
And this is used in the following functions in Unital.hs...
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Language/Drasil/Chunk/Unital.hs#L64-L82
...and their "empty list" counterparts. I'm pretty sure we could use a function that returns a DQD instead of a ConVar in all of these cases.
These are the only occurrences of ConVar or cv in Language, other than in a comment here:
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Language/Drasil/Chunk/VarChunk.hs#L47-L49
Data
ConVar is used in the following lines to define constants:
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Data/Drasil/Quantities/Chemistry.hs#L9-L11
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Data/Drasil/Quantities/Math.hs#L8-L17
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Data/Drasil/Quantities/Physics.hs#L8-L9
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Data/Drasil/Quantities/SolidMechanics.hs#L16-L17
Example
Defining Constants
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/GamePhysics/Unitals.hs#L49-L63
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/GamePhysics/Unitals.hs#L69-L86
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/SSP/Unitals.hs#L390-L441
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/SWHS/Unitals.hs#L189-L201
(This example is to be removed: )
Generates UnitalChunks
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/GamePhysics/Unitals.hs#L91-L126
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/GamePhysics/Unitals.hs#L140-L239
Forming Sentences
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/NoPCM/Body.hs#L453-L460
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/NoPCM/GenDefs.hs#L39-L46
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/SWHS/Body.hs#L1040-L1046
https://github.com/JacquesCarette/Drasil/blob/340a4de868308be50f62fe84bdfb3a531cb45184/code/Example/Drasil/SWHS/Body.hs#L1356-L1368
This was the only thing I could see a potential problem with, but since ConVar and DQD both have a ConceptChunk, and it appears that the phrase function is applied to a noun phrase through the ConceptChunk, this should be OK.
Generating a List of Contents
Sorry for the long comment, but long story short, it seems that merging ConVar and DQD shouldn't cause any problems, unless there was/were specific reason(s) for originally implementing two separate functions.
samm82
on 14 May 2018
We should begin unifying ConVar and DQD. First we should update DQD to fix the problem of dual uids. What that means is we'll want to DQD constructed in a very similar way to how ConVar is constructed. Essentially, we should have fields in our record of type:
ConceptChunkStage -> SymbolSpaceMaybe UnitDefn
Then we can use the appropriate lenses to keep our class instances.
Secondly, once that's all working, we'll move over the ConVar smart constructors and modify them to create DQDs. Then once that's working, we can remove ConVar.
szymczdm
on 14 May 2018
Another way of saying what @szymczdm said: we want to keep DQD, but want its implementation (which has problems right now) to look more like ConVar. And get rid of ConVar as a data-type.
However, keep the constructors for ConVar, just have them build the (new) DQD instead.
Then we can take a look at constructor duplication.
JacquesCarette
on 15 May 2018
One thing @szymczdm and @JacquesCarette, DQD has a QuantityDict and a ConceptChunk that both have IdeaDicts. Are they the same/is this the duplication problem? Also, what's a higher priority: this issue or #527?
samm82
on 15 May 2018
Yes, that duplication is a huge (potential) problem. Which is why this is a great opportunity to fix it. #527 is higher priority, as the results of that will influence the details of the design of this issue.
JacquesCarette
on 15 May 2018
- Would
DefinedQuantityDictremain a good name for this type? I'm asking since it no longer builds off ofQuantityDict... - Also, since it no longer builds from
Quantity,DefinedQuantityno longer is an instance ofHasAttributes; is that a problem we should be taking into consideration? Taking the following into consideration,
would it make sense to just update the current definition for DefinedQuantityDict
to have the same fields (i.e.QuantityandConceptChunk), but haveQuantityupdated to have Maybe UnitMaybe Attribs(? Or just Attribs since all the types will eventually be an instance ofHasAttributes)Maybe IdeaSpaceSymbol
niazim3
on 16 May 2018
DefinedQuantityDict is still a good name for it as it's a Quantity with a definition (regardless of the use of QuantityDict).
If Quantity has attributes, DefinedQuantityDict should also (probably) have attributes, but we'll know for sure once we've reviewed all the uses of attributes and decided what to do with them.
szymczdm
on 16 May 2018
That makes sense. And how about changing
data QuantityDict = QD { _id' :: IdeaDict
, _typ' :: Space
, _symb' :: Stage -> Symbol
, _unit' :: Maybe UnitDefn
, _attribs :: Attributes
}
to
data QuantityDict = QD { _id' :: **Maybe** IdeaDict
, _typ' :: Space
, _symb' :: Stage -> Symbol
, _unit' :: Maybe UnitDefn
, _attribs :: Attributes
}
?
niazim3
on 16 May 2018
I don't think so. Why do you want to make it a Maybe in the first place? The IdeaDict is the piece of information with the unique id and other important (mandatory) pieces of information.
szymczdm
on 16 May 2018
It didn't seem to be an overlapping feature for DQD and convar, but after looking into it again, since both currently build off of ConceptChunks, and ConceptChunks have _id :: IdeaDict, it wouldn't make sense to go forward with my proposed change.
niazim3
on 16 May 2018
The real problem / hack here is that DefinedQuantityDict uses ConceptChunk as a short-cut to get a defn, and cdom. It should just add these directly.
QuantityDict is correct as-is, and should not be changed. Only DefinedQuantityDict should be changed.
JacquesCarette
on 16 May 2018
So can we go ahead and start merging the two, and if not, is there something else we could be working on?
samm82
on 16 May 2018
Yes, go ahead. As per my last comment, please.
JacquesCarette
on 16 May 2018
And DQD should have Attributes, correct?
samm82
on 16 May 2018
Since we're no longer building off of QuantityDict, I think we can add it as a field for DQD.
niazim3
on 16 May 2018
Correct (to both).
JacquesCarette
on 16 May 2018
@JacquesCarette One thing I noticed, in the definition of DQD...
...we pass a ConceptChunk and a UnitDefn. The type of a UnitDefn is...
...and FundUnit has a ConceptChunk!
So are we passing two ConceptChunks and is this a problem?
samm82
on 16 May 2018
In this case, it is not a problem. This is because the ConceptChunk in the DQD is about the concept that the quantity embodies, while the one in the UnitDefn is the concept that the unit embodies.
But an excellent question to ask.
JacquesCarette
on 16 May 2018
@JacquesCarette Another concern: UnitalChunk is defined as follows:
DQDCV will eventually be switched over to DQD (I just had to rename it to "fix" an ambiguity error), but DQD is essentially DQDCV plus Maybe UnitDefn and Attributes.
So are DQD and UnitalChunk the same? Should we merge them too? Or am I missing something?
samm82
on 16 May 2018
They are almost the same. But DQD may have a unit, while UnitalChunk has a unit for certain.
What we probably want are two stuctures, one with a unit and one without -- but to make sure to implement the getUnit lens for the one without.
JacquesCarette
on 16 May 2018
@JacquesCarette So where should we take the UnitDefn from? Try to take it from DQD and if it's Nothing move on to UnitalChunk?
samm82
on 17 May 2018
I don't think I understand the question. You either have a DQD or UnitalChunk in your hands. You ask it for a unit, and it will give you what it has.
JacquesCarette
on 17 May 2018
@JacquesCarette Yes you're right - my bad
samm82
on 17 May 2018
So the simplest thing to do was to change DQD to Maybe have a unit. This isn't the optimal solution (as can be seen from some of the things in the examples), but it works. And it can be fixed piecemeal.
JacquesCarette
on 20 May 2018
Related issues
samm82
路
12Comments
bmaclach
路
13Comments
samm82
路
4Comments
samm82
路
3Comments
samm82
路
10Comments
Most helpful comment
In this case, it is not a problem. This is because the
ConceptChunkin the DQD is about the concept that the quantity embodies, while the one in theUnitDefnis the concept that the unit embodies.But an excellent question to ask.