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

Fix d_offset constraint in the Projectile Example #3187

Closed
smiths opened this issue Dec 19, 2022 · 8 comments · Fixed by #3192
Closed

Fix d_offset constraint in the Projectile Example #3187

smiths opened this issue Dec 19, 2022 · 8 comments · Fixed by #3192
Assignees

Comments

@smiths
Copy link
Collaborator

smiths commented Dec 19, 2022

In IM:messageIM in the Projectile Example. The input constraint says:

d_offset > - p_land

However, the constraint should be correct to read:

d_offset > - p_target

This is derived from IM:offsetIM, where we know d_offset = p_land - p_target. We also know that p_land is greater than 0. If we substitute p_land = 0 into the expression for d_offset, we find d_offset > - p_target.

This corresponds to the case where the projectile goes straight up and lands at the origin (lands at the launcher). We have constrained the launch angle to be less than pi/2, so it cannot land to the left of the launcher. The offset at the origin is -p_target. The offset cannot be smaller (more negative, further to the left) than this.

This issue was raised in #2264.

@samm82, I've assigned you to correct this, since you are the projectile expert. 😄

@samm82
Copy link
Collaborator

samm82 commented Dec 20, 2022

To make this change, I had to change the constraint in the offset unital, the messageIM input constraints, and the messageIM notes (offsetConsNote). Something smells fishy, since I had to change three areas of code to make the change completely.

I would propose populating the input constraints part of a model by getting any constraints from its inputs. As for the the notes section, maybe adding a "notes" field to a constrained object to encapsulate its rationale could work; then these notes sections could be scraped and added to any relevant models. Fixing the notes would likely be more involved, since I don't think Drasil would be able to automatically uncover the justification for a constraint, but getting constraints for a model from its inputs/outputs seems feasible to me.

@samm82
Copy link
Collaborator

samm82 commented Dec 20, 2022

This change also made no changes to the generated code, but seemed to resolve a typing issue. I'm not exactly sure what's going on with type checking, but the line "Successfully matched Rational with Float." (or its equivalent) was removed from the designLogs for projectile_s_l_nol_u_u_v_f.

@smiths
Copy link
Collaborator Author

smiths commented Dec 21, 2022

Sounds good @samm82 - both that you fixed the problem and that you critically evaluated what was involved in fixing the problem. As you said, the "Drasil way" is to make a change like this in only one place. As we discussed in #3186, in the future we would like to only have the constraint in one place. We also discussed having the option of attaching the rationale information to the constraint.

For the projectile example, I think we shouldn't repeat the constraint information and partial rationale in the Notes field. We should provide the rationale information once, probably in the Properties of a Correct Solution section, with the Table for Output Data Constraints.

@samm82
Copy link
Collaborator

samm82 commented Dec 21, 2022

@smiths Are you thinking that we should add a new column in the table or just describe the rationale in a paragraph below it? I think that a new column would be a good approach once (or if) the rationale is captured with the constraint itself, but might be too involved for a first pass.

Also in IM:messageIM is an input constraint for p_target, which does not show up in the Properties of a Correct Solution section (since it is solely an input and not an output) - where would rationale information for it go? I think it makes sense for all rationale to be together, but not if some rationale information is out of place.

@smiths
Copy link
Collaborator Author

smiths commented Dec 23, 2022

Great questions @samm82. Specifically where the rationale information goes would be something that a specific "recipe" for documenting the requirements would decide. As far as what to target initially, I think below the table would be easier than creating an additional column. Problems with an additional column will arise if the rationale has a long description. This is why we added the "details of the derivation" below the theories. If we had a field for derivation in the table for each theory, it would be too difficult to format nicely.

The rationale should go with the constraint. Since we are talking about putting output constraints below the outputs table, we could put input constraints below the inputs table. I don't think our default recipe would have a section for rationale information since there will eventually be more rationale information than just for the constraints. The "detailed derivations" are also a form of rationale.

@samm82
Copy link
Collaborator

samm82 commented Dec 23, 2022

@smiths I'm going to create a separate issue for constraint rationale. It seems that there are some pre-existing issues about rationale for constraints (#1491 and #3091) and since these issues exist in both SWHS and Projectile (and likely most if not all other examples have constraint rationale in the Notes section of their models that can be pulled out to here), this seems like a more generic fix that should be applied across the board (if a certain example doesn't have any constraint rationale, then we simply wouldn't display anything).

@samm82
Copy link
Collaborator

samm82 commented Dec 23, 2022

Created: #3197

@smiths
Copy link
Collaborator Author

smiths commented Dec 23, 2022

Thanks for digging up the old issues @samm82. Nice to see that what I say is consistent over time. (I didn't remember issue #1491 until you provided a pointer to it.) 😄 Yes, the rationale issue is cross-cutting across examples. Any nontrivial problem is going to require some rationale for the constraints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants