Fill chunk refs by Xinlu-Y · Pull Request #4434 · JacquesCarette/Drasil

Xinlu-Y · 2025-11-04T16:29:00Z

Implement real HasChunkRefs lookups for ConceptChunk, ConceptInstance, and DefinedQuantityDict so referenced UIDs flow up from definitions and short names
Rework the base chunk DB to seed the few math citations up front, drop duplicate concept chunks pulled in via DQDs, and keep citation UIDs out of the reference table
Add inReqDesc and thread it through every SRS so the input-values requirement now references the actual table; this includes injecting the table into each LabelledContent list and putting SRS.sectionReferences into allRefs
Minor cleanups triggered by the new wiring, such as reordering SI derived units and adjusting assumption lists so the generated refs resolve

JacquesCarette

There seems to be multiple logically independent sets of changes in this PR. Why all in here? It feels like this should be broken up into smaller PRs.

JacquesCarette · 2025-11-04T20:04:05Z

code/drasil-data/lib/Data/Drasil/SI_Units.hs

-derived = [becquerel, calorie, centigrade, coulomb, farad, gray, henry, hertz, joule,
-  katal, kilopascal, kilowatt, litre, lumen, lux,  millimetre, newton, ohm,
-  pascal, radian, siemens, sievert, steradian, tesla, volt, watt, weber]
+derived = [becquerel, centigrade, coulomb, hertz, joule, katal, litre,


Why have these changed order? We should make such changes unless there is a good reason to. Previous were in strict alphabetical order.

Because this change ensures every derived unit is inserted after the units it depends on.
For example, pascal precedes kilopascal, volt precedes farad/ohm, and weber precedes tesla/henry.

Drasil/code/drasil-database/lib/Database/Drasil/ChunkDB.hs

Lines 175 to 178 in e36c1ec

-- | Internal function to insert a chunk into the 'ChunkDB'. This function

-- assumes that the chunk is not already registered in the database, and quietly

-- break table synchronicity if it is.

insert0 :: IsChunk a => ChunkDB -> a -> ChunkDB

checks that all referenced units already exist when a new unit is inserted. The old order triggered missing-dependency errors.

Excellent reason, thank you! You should put in a comment to that effect. I would make sure that the base units are in alphabetical order, and then by dependency (+alphabetical) order after that.

JacquesCarette · 2025-11-04T20:06:17Z

code/drasil-example/glassbr/lib/Drasil/GlassBR/Assumptions.hs

+standardValuesDesc mainIdea = foldlSent [atStartNP' (the value), S "provided in",
+  refS $ SRS.valsOfAuxCons ([]::[Contents]) ([]::[Section]), S "are assumed for the", phrase mainIdea, 
+  sParen (ch mainIdea) `sC` S "and the", plural materialProprty `S.of_` 
+  foldlList Comma List (map (ch . view defLhs) assumptionConstants)]


take 3 was a hack, but I'm concerned that this will actually change stable?

JacquesCarette · 2025-11-04T20:08:19Z

code/drasil-example/pdcontroller/lib/Drasil/PDController/Body.hs

               Constraints EmptyS inputsUC]],

-     ReqrmntSec $ ReqsProg [FReqsSub EmptyS [], NonFReqsSub], LCsSec,
+     ReqrmntSec $ ReqsProg [FReqsSub inputValuesDescription [], NonFReqsSub], LCsSec,


I'm sure this is a good change, but it is going to lead to changes in stable. So it is usually best to do such changes in isolation (even if it means five 1-line change PRs) instead of being bundled in a larger PR.

JacquesCarette · 2025-11-04T20:09:06Z

code/drasil-example/pdcontroller/lib/Drasil/PDController/Body.hs

 -- | Holds all references and links used in the document.
 allRefs :: [Reference]
-allRefs = [externalLinkRef]
+allRefs = externalLinkRef : SRS.sectionReferences ++ map ref labelledContentWithInputs


Because this change, which is likely also good, will also change stable. We want to review these as separately as possible.

JacquesCarette · 2025-11-04T20:09:30Z

code/drasil-example/projectile/lib/Drasil/Projectile/Assumptions.hs

-assumptions = [twoDMotion, cartSyst, yAxisGravity, launchOrigin, targetXAxis,
-  posXDirection, constAccel, accelXZero, accelYGravity, neglectDrag, pointMass,
-  freeFlight, neglectCurv, timeStartZero, gravAccelValue]
+assumptions = [twoDMotion, neglectCurv, cartSyst, yAxisGravity, accelYGravity,


why the reorder?

I reorder the assumptions so that each assumption appears after any other assumptions it refers to in its description.

In Assumptions.hs, some assumptions use fromSource or fromSources to refer to other assumptions.
These functions automatically create clickable links in the generated SRS document.
However, in the old order, some of those links pointed forward to assumptions that hadn’t been listed yet, so the links appeared but didn’t work when clicked.

The new order ensures that all referenced assumptions are introduced before they are linked.
For example:

neglectCurv now comes before cartSyst

accelXZero, accelYGravity, neglectDrag, and freeFlight now come before constAccel

Great. Please add a comment to that effect. (And everywhere else you did this.)

JacquesCarette · 2025-11-04T20:10:19Z

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

 assumptions :: [ConceptInstance]
 assumptions = [assumpTEO, assumpHTCC, assumpCWTAT, assumpTPCAV, assumpDWPCoV, assumpSHECoV,
-  assumpLCCCW, assumpTHCCoT, assumpTHCCoL, assumpLCCWP, assumpCTNOD, assumpSITWP,
+  assumpLCCCW, assumpTHCCoT, assumpTHCCoL, assumpLCCWP, assumpSITWP, assumpCTNOD,


JacquesCarette · 2025-11-04T20:11:26Z

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

+-- nounPhrases instead of strings
+-- Another capitalization hack.
+progName' = commonIdea "swhsPCM" (nounPhrase''
+  (S "solar water heating systems incorporating PCM")


The first part (merging "incorporation") is fine, but inlining PCM is not a good change.

I’ll revert the inlined PCM.

JacquesCarette · 2025-11-04T20:12:03Z

code/drasil-gen/lib/Drasil/Generator/BaseChunkDB.hs

  [algorithm, errMsg, program] ++ srsDomains ++ mathcon

+basisCitations :: [Citation]
+basisCitations = [cartesianWiki, lineSource, pointSource]


Yeah, those citations are in basisCitations because the base math concepts: cartesian, line, and point already hard-code those references through fromSource.
So once any of those concepts are pulled into a ChunkDB, the matching citations have to be there too, otherwise the generator complains about missing references.

Putting them in basisCitations guarantees everything builds cleanly, but it’s a bit clunky. I have to manually update that global list. maybe there’s a better way to handle this. I’d love to hear from you.

Ah, so they're requirements of other chunks already in the basisChunkDB then? That sounds sensible to me then. This one could probably be pulled out into its own PR.

JacquesCarette · 2025-11-04T20:13:40Z

code/drasil-gen/lib/Drasil/Generator/BaseChunkDB.hs

+  where
+    baseDB = basisCDB { labelledcontentTable = idMap lc,
+                        refTable = idMap r }
+    withIdeaDicts = insertAll t baseDB


I understand why you would find this more readable... if you are going to make this change, at least line up the = signs!

I don't actually think this is really an improvement though.

The previous form made it very hard to track the registration order of different chunk types.
even a slight ordering mistake could trigger the “Missing dependency” errors I kept running into.
During this round of fixes, I repeatedly ran into such issues:

GlassBR: assumpGC referenced astm2009, but the citations were inserted later.

SWHS / SWHSNoPCM / PDController / SSP: functional requirements were registered before ReqInputs or the data-constraint tables they reference.

Projectile: constAccel cited assumptions that weren’t yet in the database.

DoublePendulum: inputValues referred to a table that hadn’t been inserted.

To fix these missing-chunk errors, I think i had to keep verifying which kinds of chunks were inserted first.

Ah, interesting. This is likely to happen again.

I think the conclusion is that inserting into the chunk database by chunk type is very fragile. We're lucky it worked at all. Please create an issue for this: I think we should change our mechanism for insertion to be more dependency-aware. But that's a much bigger change, so should not be done now. So your changes here can stay, as they'll end up being moot when this gets changed.

Xinlu-Y · 2025-11-06T19:45:58Z

I’m splitting this PR into smaller ones for easier review, stable updates will follow.

…rder issues.

…-chunk-refs

…tion sequence)

Xinlu-Y · 2025-11-13T02:49:44Z

code/drasil-docLang/lib/Drasil/DocumentLanguage.hs

Now that we use insertAllOutOfOrder12, the first call to cdb … concIns labCon … registers both the requirement (instance:inputValues) and its table (doc:ReqInputs) together, so the dependency is satisfied.
But when fillReqs runs later, it calls insertAll again for the concept instances only, without re-inserting their labelled content.
At that point insertRefExpectingExistence looks in the chunkTable for doc:ReqInputs, can’t find it, and throws error.

So here I just skip fillReqs when the system already provides its own requirements to avoid the duplicate insertion and keep the dependency check happy.

As a result, the requirement order in the chunk table becomes existing FRs first, followed by new NFRs

Xinlu-Y · 2025-11-13T03:08:44Z

This PR currently includes too many commits, including merges and some testing from @balacij‘s branch. I’ll split them up later to make the review easier.

balacij · 2025-11-18T20:07:17Z

code/drasil-database/lib/Database/Drasil/ChunkDB.hs

+    -- Calculate what chunks are depended on (i.e., UID -> Dependants), but only
+    -- keep dependencies that correspond to chunks we are actually inserting or
+    -- that already exist in the chunk table. References/LabelledContent live in
+    -- separate tables and are handled elsewhere.
+    allowedDeps = S.fromList (M.keys (chunkTable strtr) ++ map (^. uid) calt)
+    chDpdts = invert $ M.fromList $
+      map (\c -> (c ^. uid, S.toList $ S.filter (`S.member` allowedDeps) (chunkRefs c))) calt


I'm not quite sure I'm understanding this change. Does it not make it so that any chunk dependancy that is neither already in the ChunkDB nor in the list of chunks being added, is ignored? I think this would mean that we can insert chunks into the ChunkDB that have unsatisfied dependencies?

balacij · 2025-11-18T20:08:44Z

code/drasil-example/dblpend/lib/Drasil/DblPend/Body.hs

 -- | Holds all references and links used in the document.
 allRefs :: [Reference]
-allRefs = [externalLinkRef]
+allRefs = externalLinkRef : SRS.sectionReferences ++ map ref (labelledContent ++ funcReqsTables)


Was this really needed? The fillcdbSRS function should be doing this already for any LabelledContent registered in the ChunkDB. (I'm not saying it should be doing that, only that it currently does that.)

balacij · 2025-11-18T20:17:37Z

code/drasil-lang/lib/Language/Drasil/Chunk/Concept/Core.hs

+  chunkRefs ci =
+    let conceptRefs    = chunkRefs (ci ^. cc)
+        shortNameRefs  = collectSentenceRefs (getSentSN (shortname ci))
+        domainRefs     = S.fromList (cdom ci)
+    in conceptRefs `S.union` shortNameRefs `S.union` domainRefs


This is really good! And your code now makes me think: Can we instantiate HasChunkRefs for Sentence? I think that would help simplify your code and make it so that you don't need to import anything here. It would only be union of the 3 lists of chunkRefs -- e.g.,

chunkRefs (ci ^. cc) `S.union` (S.fromList (cdom ci)) `S.union` chunkRefs (ci ^. defn) ... (elsewhere)... instance HasChunkRefs Sentence where chunkRefs s = ...

?

Now this makes me think: if all of this code is going to be more or less following the same structure, I think we can use deriving-via to auto-generate these instances.

Xinlu-Y added 17 commits October 28, 2025 10:53

gather chunk refs from Reference metadata

d3c5451

HasChunkRefs for IdeaDict

f930e6b

HasChunkRefs for Citation

32f3c04

HasChunkRefs for UnitDefn

e9e0038

collect chunk references from concept metadata

d28cafb

seed basis citations and filter duplicates

5719827

feat(glassbr): track input references

8875f1f

feat(dblpend): surface input properties requirement

631eec3

feat(pdcontroller): capture controller input table

fd37057

feat(projectile): capture launch inputs and tidy assumptions

3cd0f38

feat(gamephysics): expose section reference citations

b926a6a

feat(projectile): capture launch inputs and tidy assumptions

fdd1c54

feat(ssp): treat input table as explicit requirement

517ff62

expose input requirement sentence builder

97212b7

feat(swhs): register input table and clean metadata

a56504d

feat(swhsnopcm): wire input table into requirement trace

f9eda18

feat(sglpend): surface pendulum input requirement

9db8448

Xinlu-Y mentioned this pull request Nov 4, 2025

Drasil Team Meeting - Tuesday, Nov 4, 1:45 pm, ITB/112 #4427

Closed

JacquesCarette requested changes Nov 4, 2025

View reviewed changes

Xinlu-Y mentioned this pull request Nov 11, 2025

ChunkDB insertion order is fragile: dependency checks fail if chunks are registered out of sequence #4441

Closed

Xinlu-Y added 6 commits November 11, 2025 10:27

revert the inlined PCM

288381d

add comments about reordering the assumptions

5ba7073

add comments about reordering the units

21e5fde

Register phsChgMtrl before program name entries to fix dependency o…

65cc135

…rder issues.

regenerate stable reference sections with shared citations

d82f5c4

resequence assumptions and trace artifacts after chunk reorder

ea3a593

Xinlu-Y mentioned this pull request Nov 11, 2025

Drasil Team Meeting - Tuesday, Nov 11, 1:45 pm, ITB/112 #4440

Closed

Xinlu-Y added 2 commits November 11, 2025 12:32

Merge remote-tracking branch 'origin/main' into fill-chunk-refs

2d418fa

fix chunk ref collection for new NPStruct

30a1152

Xinlu-Y added 8 commits November 12, 2025 19:38

Merge remote-tracking branch 'origin/cdbInsertOutOfOrder12' into fill…

7d410dc

…-chunk-refs

cleaned up the unused citation imports

40a86f6

rewrite fillReqs to skip registered requirements

ec610c9

remove unital symbols and add loadDur as a defined quantity.

e70066f

stabilized outputs (trace table reordering due to updated chunk inser…

b8a2991

…tion sequence)

Merge remote-tracking branch 'origin' into fill-chunk-refs

1b11b06

fix name shadowing in DefinedQuantity chunkRefs

3028ab6

remove redundant import

e3b3de0

Xinlu-Y commented Nov 13, 2025

View reviewed changes

Xinlu-Y added 6 commits November 18, 2025 10:10

Merge remote-tracking branch 'origin/main' into fill-chunk-refs

647c79b

remove unused chunks from ChunkDB

84e07b2

align examples with inReqWTab API

5e39b6b

fix trailing whitespace

2b0c445

stabilize

f59b91a

remove deprecated multilingual field from mdBook config

4068616

Xinlu-Y marked this pull request as ready for review November 18, 2025 16:12

Xinlu-Y requested review from bmaclach, cd155, samm82 and smiths as code owners November 18, 2025 16:12

Xinlu-Y mentioned this pull request Nov 18, 2025

Drasil Team Meeting - Tuesday, Nov 18, 1:45 pm, ITB/112 #4442

Closed

balacij reviewed Nov 18, 2025

View reviewed changes

This was referenced Nov 20, 2025

declareHasChunkRefs: Generate instances of HasChunkRefs where possible #4476

Merged

remove deprecated multilingual field from mdBook config #4491

Merged

This was referenced Dec 2, 2025

Drasil Team Meeting - Tuesday, Dec 2, 1:30 pm, ITB/112 #4496

Closed

Haschunkrefs sentence #4514

Open

Xinlu-Y mentioned this pull request Dec 9, 2025

Haschunkrefs np #4515

Merged

	-- \| Internal function to insert a chunk into the 'ChunkDB'. This function
	-- assumes that the chunk is not already registered in the database, and quietly
	-- break table synchronicity if it is.
	insert0 :: IsChunk a => ChunkDB -> a -> ChunkDB

Conversation

Xinlu-Y commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JacquesCarette left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xinlu-Y Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xinlu-Y commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xinlu-Y commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Xinlu-Y commented Nov 4, 2025 •

edited

Loading

Xinlu-Y Nov 6, 2025 •

edited

Loading