Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Duplicate "the" (coming from the_ofThe, etc.?) #3234

Closed
samm82 opened this issue Jan 10, 2023 · 9 comments · Fixed by #3479
Closed

Duplicate "the" (coming from the_ofThe, etc.?) #3234

samm82 opened this issue Jan 10, 2023 · 9 comments · Fixed by #3479
Assignees
Labels
easy At quick glance, it should be 'easy,' but you never know what'll happen! stable-fix

Comments

@samm82
Copy link
Collaborator

samm82 commented Jan 10, 2023

From my investigation in #3199, I noticed some UnitalChunks with sentences built using the_ofThe, etc. but also having "the" prepended (these specific examples from DblPendulum):

xVel_1 = makeUCWDS "v_x1" (nounPhraseSent $ phraseNP (horizontalVel `the_ofThe` firstObject))
(S "The" +:+ phraseNP (QP.angularVelocity `the_ofThe` firstObject) `S.inThe` phrase CM.xDir)
(sub lV (Concat [labelx, label1])) velU

yVel_1 = makeUCWDS "v_y1" (nounPhraseSent $ phraseNP (verticalVel `the_ofThe` firstObject))
(S "The" +:+ phraseNP (QP.angularVelocity `the_ofThe` firstObject) `S.inThe` phrase CM.yDir)
(sub lV (Concat [labely, label1])) velU

lenRod_1 = makeUCWDS "l_1" (nounPhraseSent $ phraseNP(len `the_ofThe` firstRod))
(S "The" +:+ phraseNP (len `the_ofThe` firstRod)) -- Fix me, can have more information
(sub cL label1) metre

pendDisAngle_1 = makeUCWDS "theta_1" (nounPhraseSent $ phraseNP (angle `the_ofThe` firstRod))
(S "The" +:+ phraseNP (angle `the_ofThe` firstRod))
(sub lTheta label1) radian

I believe this results in "the" being duplicated:
image

@samm82 samm82 added stable-fix easy At quick glance, it should be 'easy,' but you never know what'll happen! labels Jan 10, 2023
@JacquesCarette
Copy link
Owner

Nicely spotted!

@cd155
Copy link
Collaborator

cd155 commented Jan 14, 2023

The problem occurs while using the combination of phraseNP and "the_ofThe".

For example,
nounPhraseSent $ phraseNP (horizontalVel `the_ofThe` firstObject)

the_ofThe will add "the" at the beginning, but the whole word is treated as an NP. The second "the" will be added when we display the description in the definition.

If we change the the_ofThe to ofThe, will remove the extra "the" from the definition. However, this will also remove "The" from the Table of Symbols.

@JacquesCarette
Copy link
Owner

This code seems to be quite cursed: it really visibly adds the twice. Worse, it does its own capitalization. And it duplicates things too.

@smiths
Copy link
Collaborator

smiths commented Jan 25, 2023

At the risk of glossing over a more serious issue, I'm completely fine with the last idea from @cd155. That is, using ofThe is fine with me. The consequence is removing the "The" from the Table of Symbols, but that "The" isn't really necessary. If we say vx1 is the "horizontal velocity of the first object" that is fine with me. We don't have to say "The horizontal velocity of the first object". I looked at the GlassBR example and it isn't full of "the"s in the table of symbols. Including a "The" in the definition of the symbol seems like unnecessary information, especially since we can add a "the" if we want to with the combinators.

@samm82
Copy link
Collaborator Author

samm82 commented Jan 26, 2023

I agree with @smiths here; making our descriptions more consistent is a good idea, especially if it also works to remove errors elsewhere. It will be interesting to see the implications of this change!

@JacquesCarette How do you propose handling the duplication? Define a local Sentence (like lenRod_1Desc) and use it twice? We could potentially introduce a UnitalChunk constructor for simple terms (where the term is the description), but means creating more constructors, which could potentially introduce unwanted complexity (#3199).

@JacquesCarette
Copy link
Owner

Yes, at this point, something as simple as lenRod_1Desc would do. Let's not introduce more constructors, but see what we do in practice first.

@janim2-2004 janim2-2004 self-assigned this Jun 12, 2023
@janim2-2004
Copy link
Collaborator

@samm82 I am not sure why we use

nounPhraseSent $ phraseNP(len `the_ofThe` secondRod)

instead of just

(len `the_ofThe` secondRod)

@samm82
Copy link
Collaborator Author

samm82 commented Jun 12, 2023

Just by looking at the code, me neither. I can't verify that the types all match up with 100% certainty, but if everything works using the second approach, I agree that it is much nicer

@cd155
Copy link
Collaborator

cd155 commented Jun 12, 2023

Practically, you can change nounPhraseSent $ phraseNP(len `the_ofThe` secondRod) to len `the_ofThe` secondRod, then try to compile Drasil, use stable to see what makes changes in Latex and HTML. If nothing changes, then it looks simpler.

@janim2-2004 janim2-2004 added stable-fix easy At quick glance, it should be 'easy,' but you never know what'll happen! and removed stable-fix easy At quick glance, it should be 'easy,' but you never know what'll happen! labels Jun 12, 2023
This was referenced Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy At quick glance, it should be 'easy,' but you never know what'll happen! stable-fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants