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

Automatically update fields in the database #2775

Open
Ant13731 opened this issue Aug 9, 2021 · 5 comments
Open

Automatically update fields in the database #2775

Ant13731 opened this issue Aug 9, 2021 · 5 comments
Assignees
Labels
needs-clarification Needs a clear 'state', 'goal', 'analysis', and 'explanation' to reduce solution ambiguities.

Comments

@Ant13731
Copy link
Collaborator

Ant13731 commented Aug 9, 2021

Building off of #2675 and #2774, it might be nice to include all "complex" (or higher-level) chunks in their reduced forms for other fields of the chunk database. For example, everything that is in the ConceptMap field of a ChunkDB can be included under the TermMap, since the ConceptChunks used in a ConceptMap contains IdeaDicts which can be used in the TermMap. This reduces the need for a user to include the same chunk in both fields.

See #2675 for the original discussion on this issue.

@Ant13731 Ant13731 self-assigned this Aug 9, 2021
Ant13731 added a commit that referenced this issue Aug 9, 2021
@Ant13731
Copy link
Collaborator Author

Ant13731 commented Aug 9, 2021

When trying to include all chunks that are a part of the NamedIdea class as an IdeaDict, some examples have chunks that use the same UID with different terms. For example, in 30b4e1f, running make with the filler for IdeaDicts results in some unwanted, stable-breaking changes in GamePhysics and SSP. In particular, GamePhysics' overuse of the accj UID is really prominent in both the log files (debug/GamePhysics_SRS) and stable. I don't think we want this, so I'll list the terms that need fixing here:

UID Example Issues
accj GamePhysics the term used for the concepts and instance models conflicts with the one used for QuantityDict
mobShr and mobilizedShear SSP General definition term for mobShr conflicts with the concept version. The underlying issue for this is that there should not be manual pluralization of the term since it is a nounphrase.

(The above should be fixed before completing this issue).

Edit: Related to #2371

Note: There have been a lot of other PRs and issues on the topic of conflicting UIDs, most of them are closed though.

This kind of problem stems from some constructors that override the term from a given chunk while inheriting the same UID from that chunk. Specifically, those constructors for ModelKind found in drasil-theory should be looked at. @balacij and I discussed this a bit further and found that we could temporarily solve this by appending MK to the UID whenever it is inherited without its term. Or, we could just append the kind of model to the end of the UID whenever it gets used (so DataDefinitions get DD, GenDefs get GD, etc.), which would also help users look for these model-level chunks in the chunk logs (ex. just CTRL+F the type you want).

But @balacij also pointed out that at that point, we are reducing a clear line of information to just a string for use in a UID and that using strings for UIDs may not be the best option.

It could also be argued that we may also want to just keep these fields separate in the chunk database so they don't conflict. But it may be nice to refer to the term of a model-level chunk in a Sentence, so we may want the flexibility of combining the fields. Perhaps we shouldn't even allow different terms for the same UID in the first place.

@Ant13731
Copy link
Collaborator Author

Looking at the GamePhysics issue in a little more depth, I found for the IMod rotMot the following description of the variables:
image
Should the a_j variable description be replaced with "a_j is the acceleration of the jth body"? The term is already attached, just overwritten as of now.

@smiths
Copy link
Collaborator

smiths commented Aug 12, 2021

Good catch @Ant13731. You are correct that alpha_j is not the force. It is the angular acceleration. We should be clear that it is related to the rotational motion, not the translational motion. Can you change it to "alpha_j is the angular acceleration of the j-th body (rad/s^2)". Interestingly, the units were correct in the original; it is just the definition that is wrong.

@Ant13731
Copy link
Collaborator Author

#2788 should be completed before uncommenting the functions that link between ChunkDB fields

@JacquesCarette
Copy link
Owner

The basic idea is very good - the chunk hierarchy should indeed be used, instead of forcing the work onto the 'user'.

I'm not so happy with the implementation scheme. I would prefer to teach Drasil about its own hierarchy, and thus have the lookup functions do the work automatically.

That still leaves the issue of using the same UID for multiple things. In any single example, that should not be allowed at all.

This kind of problem stems from some constructors that override the term from a given chunk while inheriting the same UID from that chunk.

That does indeed seem bad. I think I remember why: we're specializing from something generic to something more specific. Having a more specific term is good -- re-using the same UID is where things go wrong.

@balacij balacij added the needs-clarification Needs a clear 'state', 'goal', 'analysis', and 'explanation' to reduce solution ambiguities. label Apr 27, 2023
@balacij balacij self-assigned this Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-clarification Needs a clear 'state', 'goal', 'analysis', and 'explanation' to reduce solution ambiguities.
Development

No branches or pull requests

4 participants