Automatically collect abbreviations from generated DocDescs#4363
Automatically collect abbreviations from generated DocDescs#4363sarrasoussia wants to merge 66 commits intomainfrom
DocDescs#4363Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| "|DD|Data Definition|\n", | ||
| "|GD|General Definition|\n", | ||
| "|GS|Goal Statement|\n", | ||
| "|GamePhysics|GamePhysics|\n", |
There was a problem hiding this comment.
I don't think GamePhysics should be treated as an abbreviation.
There was a problem hiding this comment.
progName :: CI
progName = commonIdeaWithDict "gamePhysics" (pn "GamePhysics") "GamePhysics" [physics]GamePhysics (and the other examples you mentioned) all have code like this above because we require each of our case studies to have an abbreviation/codename.
So, should we loosen said restriction and remove the false abbreviation given to GamePhysics and co.? Or should we write a rule that filters out abbreviations in the table if they're equivalent to the terms. Or something else entirely?
Or should we write a rule that filters out abbreviations in the table if they're equivalent to the terms.
This might be a good rule to impose on our IdeaDicts, CIs, etc. in general.
There was a problem hiding this comment.
An abbreviation is not a a 'codename'. If we require codenames, we should have a different data-structure for those.
We should not conflate different concepts.
There was a problem hiding this comment.
Going back to this, while I agree with you, I think this is out of scope for this PR. We should fix it in another. I'm okay with this PR being blocked on that, but I'm also okay with pushing it to later in the name of merging in this PR (which is already quite large and a bit difficult to keep up to date with main).
There was a problem hiding this comment.
Good: I think we should indeed fix that first.
I'm thinking a new ProgramName chunk, with fields name (or label) and some optional description. It should not implement the term lens. We're overloading CI too far here.
| "<a id=\"Sec:LCs\"></a>\n", | ||
| "\n", | ||
| "This section lists the likely changes to be made to the software.\n", | ||
| "This section lists the Likely Changes (LC) to be made to the software.\n", |
There was a problem hiding this comment.
likely changes should not be capitalized here (same with Unlikely Changes below)
There was a problem hiding this comment.
Do we not always want to capitalize noun phrases when we introduce their acronyms?
There was a problem hiding this comment.
I don't think so - but we should ask @smiths for another opinion.
There was a problem hiding this comment.
I have always thought that capitalization should be used for introducing acronyms, but I recently learned that the rule is to capitalize the words in expanded form if the full phrase is a proper name or a formal title, like NASA. If the expanded phrase is not a proper noun, then the individual words are not capitalized, like research software engineer (RSE). According to ChatGPT, the APA, AMA, Chicago and IEEE all say this.
Do we have any way of knowing in our Drasil data whether the expanded form of our acronyms is a proper noun or not?
There was a problem hiding this comment.
In theory, we have a way of knowing whether something is a proper noun or not. In practice, it has been abused, so it is not currently a proper source of 'truth'. But we could make it so.
Thanks for justifying that the individual words should not be capitalized.
| <td>Glass Type Factor</td> | ||
| </tr> | ||
| <tr> | ||
| <td>GlassBR</td> |
There was a problem hiding this comment.
Similarly, GlassBR should not be treated as an abbreviation.
| <td>proportional derivative</td> | ||
| </tr> | ||
| <tr> | ||
| <td>PD Controller</td> |
There was a problem hiding this comment.
Same "PD Controller" should not be an abbreviation
| <td>Physical System Description</td> | ||
| </tr> | ||
| <tr> | ||
| <td>Projectile</td> |
There was a problem hiding this comment.
Projectile should not be an abbreviation
| "|Refname|Reference Name|\n", | ||
| "|SRS|Software Requirements Specification|\n", | ||
| "|SWHS|Solar Water Heating System|\n", | ||
| "|SWHS|Solar Water Heating Systems Incorporating PCM|\n", |
There was a problem hiding this comment.
SWHS now has two meanings!
There was a problem hiding this comment.
Agreed, but, similarly, I think this is out of scope for this PR. We will need to file an issue if this PR is approved+merged.
I'm not sure why this was removed to begin with. Bad merge?
ab3e162 to
e5e628f
Compare
nouns. Follows @smiths' comment on #4363: #4363 (comment)
nouns. Follows @smiths' comment on #4363: #4363 (comment)
|
I think I'm going to call this PR "Ready for Review," @JacquesCarette @smiths. Regarding the paragraph previously added through this PR, as I mentioned in #4363 (comment), I've removed it. We should fix the root issue here in a subsequent PR. For now, I think this PR is fine enough. There is the issue of the "code name"s being added to the table of abbreviations and acronyms, but I think we can file an issue to deal with this later. |
smiths
left a comment
There was a problem hiding this comment.
I looked at the changes to stable. The changes look reasonable to me.
|
There's a conflict to resolve. |
| mkSections si dd = map doit dd | ||
| mkSections si dd = | ||
| let | ||
| splitByTAandA :: [[DocSection]] |
There was a problem hiding this comment.
I think this code should be refactored into even more generic kit. What you are doing is creating a 'view' (in this case, zooming in to RefSec), mapping onto one side of that view, and then putting everything back together.
A more refined way to do this would take a function (a -> Either b c) instead of a predicate, then two functions (b -> d) and (c -> d) for each side, then you could unEither. In the case below, it would actually be (a -> Either b a) and the function on the False stuff would be id.
| @@ -126,10 +126,10 @@ sentencePlate f = appendPlate (secConPlate (f . concatMap getCon') $ f . concatM | |||
| (GDs s _ d _) -> def d ++ s ++ der d ++ notes d | |||
| (IMs s _ d _) -> s ++ der d ++ notes d | |||
There was a problem hiding this comment.
Hmm, this part is still not addressed?
Closes #4299
Based on #4323
Builds on #4695