Conversation
|
I think this PR uncovered some mistakes in the derivatives that weren't found because the scale_performance flag was false. |
| warnings.warn( | ||
| f'EngineDeck <{self.name}>: aircraft:engine:scaled_sls_thrust and ' | ||
| 'product of aircraft:engine_scale_factor and ' | ||
| 'aircraft:engine:reference_sls_thrust are not an exact match but within ' | ||
| 'tolerance. Setting scaled thrust target to calculated value of ' | ||
| f'{target_thrust} lbf.' | ||
| ) |
There was a problem hiding this comment.
I think we should simplify this and go with the following logic:
If both SCALED_SLS_THRUST and SCALE_FACTOR are provided warn the user, ignore one and use the other (maybe sscale_factor should take precedence)?
This eliminates needing to do a tolerance check, and Aviary's logic is clear: it uses scale_factor to scale the engine. If scale factor is not provided it will be calculate based on SCALED_SLS_THRUST and reference thrust.
I think this warning as written will always trigger if thrust_provided = True, even if the the scaling is an exact match? If we want this warning then we should add it under an additional 'if' checking for within tolerance but not exact? Personally I think this is unecessary - if the provided scaling matches to within 1e-4 that's 0.01% and more than accurate enough to be ignored.
There was a problem hiding this comment.
You shouldn't ignore one because SCALED_SLS_THRUST is later used for calculations, so if it doesn't match scale factor it gets overwritten to a "incorrect" value. It's either check for a match or always error when both are provided.
There was a problem hiding this comment.
I made it a DEBUG message for now
| @@ -1364,44 +1320,34 @@ def _set_reference_thrust(self): | |||
| # both scale factor and target thrust provided: | |||
There was a problem hiding this comment.
As described below I think we should simplify this block of code for if we have both.
I would throw a warning stating both have been provided and so scaled_sls_thrust will be ignored.
Then I would move to the 'scale_factor has been provided, no scaled_thrust provided' logic block so Aviary is consistent and remove the complexity of the tolerance and warnings etc.
There was a problem hiding this comment.
As mentioned before, I am not sure if it is safe to ignore SLS thrust value provided. I think I will put a deprecation warning here that in the future we will not allow specifying both, with the plan to only accept scale factor in the future.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Summary
Removes unnecessary variable Aircraft.Engine.SCALE_PERFORMANCE
Related Issues
Backwards incompatibilities
None
New Dependencies
None