Replacing explicit ShortName with Label in Chunk types

[elwazana]

Chunk

• AssumpChunk
• Change
• Concept/Core !
• Citation !
• Eq
• GenDefn
• Goal !
• InstanceModel
• PhysSystDesc !
• Relation !
• ReqChunk
• Theory

Document

• Section
• Contents

In PR #693 @JacquesCarette and I discussed replacing the explicit shortname in some Chunk types with a Label, I've created this issue just to keep track of which ones need to be replaced.

Also as a note the (!) next to some are due to them having an instance of HasShortName but posses no ShortName, i.e:

@JacquesCarette Should I add a Label to these as well?

[JacquesCarette]

Goal and PhysSystDesc should definitely have labels.

Relation is a horrible hack. But that is a separate thing. The way it is abused right now means that it should likely have a label as well.

If a Citation is a bibliography entry, then yes, that too should have a label.

[elwazana]

@JacquesCarette Hello Dr. Carette, I missed Concept/Core.hs when I was creating the task list above. Does that need a Label as well? It currently has a Maybe Shortname.

[JacquesCarette]

Can you figure out when the shortname of a Concept is used? I mean, when is it set (in examples). I think that will help me understand its usage, which I think will guide the design.

[Mornix]

ConceptChunk gained the Maybe ShortName last week as part of the work to de-embed ALUR (and make ConceptChunk referable.
It is currently unused. All constructors at the moment simply set it as Nothing.

[JacquesCarette]

Right. That should be upgraded to a Maybe Label. We can refine later, but at least that should allow both @elwazana and @Mornix to proceed.

[elwazana]

@JacquesCarette Hello Dr. Carette, I've tried incorporating Maybe Label but whenever I get to the instance of HasLabel I can't figure out how to use a Lens' to unwrap the Maybe from the Maybe Label.

I've looked through the other types to see if a Lens was used over a Maybe type and the only way around this I could find is to create a:

class HasMaybeLabel c where
  getMaybeLabel :: Lens' c (Maybe Label)

But this solution seems bit smelly to me, is there another solution? If not can I get a go ahead on this solution?

[JacquesCarette]

No, that's right, you have to create a Lens to a Maybe Label.

[elwazana]

@JacquesCarette I implemented the above to correct the Maybe Label error in the instance of HasLabel, after that, I tried to fix the HasShortName instance for ConceptChunk so that it retrieves the shortname from the Label. When I did that I got this error:

So to fix that I added an instance of HasShortName specifically for Maybe Label:

But this resulted in this error:

I've discussed possible solutions with @niazim3 but nothing we come up with seems to even change the error.

[JacquesCarette]

So you won't be able to build a proper instance of HasShortName for ConceptChunk out of getMaybeLabel. This is because it might be Nothing, which is not a valid shortname!

In other words, the handling of label/shortname for ConceptChunk is going to have to be coded differently. Right now, since it's not used, simply don't worry about it, we can let @Mornix deal with it later.

[niazim3]

Is there an example of a time where a Label would not be desirable when building a ConceptChunk? If not, can Concept/Core.hs take a Label instead of a Maybe Label?

[Mornix]

From when I was looking at the currentConceptChunk uses, it looks like the type backs a lot of things in Data.Drasil and I believe more localized concepts in each individual example. These currently aren’t referable. (I’m not sure if they should be referable in the future /cc @JacquesCarette)

[JacquesCarette]

Right. ConceptChunk is very basic in our hierarchy, and often corresponds to purely internal knowledge that does not necessarily even appear in documents. Those do not need to be referable.

Eventually I think we will end up splitting our uses of ConceptChunk into more meaningful sub-categories, and then some of the sub-categories will have labels, and the others will not. But we're not quite ready for that yet.

[niazim3]

Would it be a good idea to work on the other data types as discussed in previous issues and meetings in the meanwhile (ignoring the Maybe problem brought up by Concept/Core)?

Perhaps another issue can be made for that as this is commented out (to ensure the implementations for the other data types are correct)?

Or is this issue being in the midst of being worked on/close to being finished that we can wait on completion of this whole issue?

[JacquesCarette]

Yes, getting many of the data-types which need this work done now is a good idea.

ConceptChunk is not close enough to rely on. So it would be best if this could proceed independently.

[elwazana]

@JacquesCarette Since we will be removing the instance for Contents is it necessary to update the HasShortName instances of Contents to match shortname becoming a Lens instead of a getter?

[elwazana]

@JacquesCarette Also with regards to Section:

Would it make more sense to have Section as a record type? Or should it be kept as is for the design?

[JacquesCarette]

We discussed removing the instance for Concept yesterday. Not Contents.

Although grabbing a label from a Contents might eventually make more sense than directly getting the shortname.

Yes, it makes sense to make Section a record type, as that can make accessing parts easier.

[elwazana]

@JacquesCarette I was referring to issue #629 task 4 of @niazim3 checklist when I mentioned removing the instances of Contents, unless that has been changed?

[JacquesCarette]

Ah, that makes sense. Right, Content will be 'raw' and LabelledContent will have the Label and Content will not. I had forgotten - perfect.

[elwazana]

@JacquesCarette Hello Dr. Carette, I've removed the instances of Contents and I had a question about what should be done with the referencing functions in References.hs:

There are two issues here:

1. acroTest and find' use makeRef to work, however, makeRef requires a type with a HasShortName and Referable instance which Contents doesn't have anymore.

2. With regards to find' should it be changed to:

find' :: LabelledContent -> [LabelledContent] -> LabelledContent
find' = ~~~~~

Also, I realize that acroTest will be replaced alongside the other acro- functions but for now what should be done with it?

[niazim3]

1. Re: Since LabelledContent is now an instance of HasShortName and Referable, I think it can replace Contents.

[JacquesCarette]

I agree with @niazim3 , it does seem like LabelledContent should fit the bill.

[elwazana]

@JacquesCarette Hello Dr. Carette I was able to integrate LabelledContent in place of Contents for the referencing functions and that runs fine now. But I've run into this problem that I'm unsure how to solve.

The problem is that convertRel uses ec which is a constructor for Eq, this was adjusted to take in a Label instead of a ShortName, but fixing that here to take a Label causes the following to break:

I was able to fix the issue by doing this:

However, this seems a bit off to me. I'm not sure if this is becuase relToQD is a hack (issue #628), or its becuase of my fix. So I just wanted to get some feedback on it before getting too far ahead.

[JacquesCarette]

Both relToQD and convertRel are awful hacks. Your change is 'optimal' given the constraints.

The correct fix involves ripping out ExprRelat altogether and creating a proper class for 'definitions'. Right now, our definitions are done implicitly through the relational subset of Expr, and is super-fragile. However, it will be needed once we move to doing code generation for ODEs, for example.

It would be nice if we could get to at least fixing the relToQD hack this summer.

[niazim3]

Note:

As of the most recent commit, build is breaking at the following error:

.\code\Data\Drasil\Utils.hs:139:18: error:
    • No instance for (HasShortName Contents)
        arising from a use of ‘makeRef’
    • In the first argument of ‘(.)’, namely ‘makeRef’
      In the expression: (makeRef . Definition . f)
      In an equation for ‘refFromType’:
          refFromType f = (makeRef . Definition . f)

. It originally looked like all that needs to be done is convert the Contents created with the call on Definition to a LabelledContent (may still be the case).

This refFromType is used in the examples on [QDefinitions] and [RelationConcept] (e.g.

Drasil/code/Example/Drasil/GlassBR/Body.hs

Lines 617 to 623 in b3938fb

	traceability_matrices_and_graphs_theorysRef = map (refFromType Theory) tModels
	traceability_matrices_and_graphs_instaModel = ["IM1", "IM2", "IM3"]
	traceability_matrices_and_graphs_instaModelRef = map (refFromType Theory) iModels
	traceability_matrices_and_graphs_dataDef = ["DD1", "DD2", "DD3", "DD4", "DD5", "DD6", "DD7", "DD8"]
	traceability_matrices_and_graphs_dataDefRef = map (refFromType Data) dataDefns

) to tack on "DD" or "T" (depending on if the element Definition is a Theory or Data):

Drasil/code/Language/Drasil/Reference.hs

Line 220 in b3938fb

refAdd t = "T:" ++ t ^. uid

Drasil/code/Language/Drasil/Reference.hs

Line 230 in b3938fb

refAdd d = "DD:" ++ concatMap repUnd (d ^. uid)

, since all makeRef is expecting is anything that is an instance of HasShortName and Referable, which anything with a Label will be.

If RelationConcept becomes an instance of HasLabel, there will be no need for any conversion to a Contents or LabelledContent or for appending "T:" or "DD:" to any labels, which is something good to remove (seeing #734).

[niazim3]

Question/Proposal

Having said that, #628 mentions that RelationConcept is too raw/unstable to be augmented; therefore, TMs, IMs, and GDs should build off of RelationConcept (which InstanceModel and GenDefn already does:

Drasil/code/Language/Drasil/Chunk/InstanceModel.hs

Lines 32 to 44 in b3938fb

	-- | An Instance Model is a RelationConcept that may have specific input/output
	-- constraints. It also has attributes like derivation, source, etc.
	data InstanceModel = IM { _rc :: RelationConcept
	, _imInputs :: Inputs
	, _inCons :: InputConstraints
	, _imOutput :: Output
	, _outCons :: OutputConstraints
	, _ref :: References
	, _deri :: Derivation
	, _refName :: ShortName
	, _notes :: Maybe [Sentence]
	}
	makeLenses ''InstanceModel

Drasil/code/Language/Drasil/Chunk/GenDefn.hs

Lines 17 to 24 in b3938fb

	-- | A GenDefn is a RelationConcept that may have units
	data GenDefn = GD { _relC :: RelationConcept
	, gdUnit :: Maybe UnitDefn
	, _deri :: Derivation
	, _ref :: References
	, _refName :: ShortName
	}
	makeLenses ''GenDefn

).
@szymczdm @JacquesCarette Is there a reason TheoryChunk doesn't

Drasil/code/Language/Drasil/Chunk/Theory.hs

Lines 32 to 42 in b3938fb

	data TheoryChunk = TC { _tid :: UID
	, _vctx :: [TheoryChunk]
	, _spc :: [SpaceDefn]
	, _quan :: [QuantityDict]
	, _ops :: [ConceptChunk]
	, _defq :: [QDefinition]
	, _invs :: [TheoryConstraint]
	, _dfun :: [QDefinition]
	, _ref :: References
	}
	makeLenses ''TheoryChunk

? Should it first be reworked to build off of a RelationConcept before continuing the work on LabelledContent?

[JacquesCarette]

TheoryChunk is good at it is. It is a collection of lots of things.
GenDefn and InstanceModel are pretty good, but still a bit hacky. Mostly because RelationConcept is somewhat broken. It should really be a generalization of QDefinition. This hack is similar to the one afflicting relToQD.

[JacquesCarette]

Regarding the previous comment: even if RelationConcept is sub-optimal (but just in the 'fact' that it has a relation instead of a defining equation, which it usually has, but encoded as a relation), it is still the right 'base' on which to build lots of stuff. And thus adding a Label to it is a good idea.

[niazim3]

If a Label is added to RelationConcept, would that Label be propagated to the InstanceModel and GenDefns that build off of the RelationConcept?
Or would those data instances have their own Label?
@szymczdm @JacquesCarette

[JacquesCarette]

I think that they would be separate labels. Hmm, I think I was assuming Maybe Label for RelationConcept, not just Label.

[JacquesCarette]

Just a note for @Nathaniel-Hu and @muhammadaliog3 that your work of finding all the data, classes and instances will have an impact on this issue. The underlying reason it's still not fixed is that our concept matrix isn't quite "right".