-
Notifications
You must be signed in to change notification settings - Fork 119
feature:extend sideview vertical range to 99km / Fix #2935 #2936
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
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good so far, but how does I know it's limited to 0-99 km? For example, if I want to change 98.5 to 99.5, it doesn't allow me to until I remove the decimal places.
Maybe all larger values should become replaced by its upper limit.
| assert thermolib.isa_temperature(51000 * units.m).magnitude == pytest.approx(270.65) | ||
| assert thermolib.isa_temperature(71000 * units.m).magnitude == pytest.approx(214.65) | ||
| assert thermolib.isa_temperature(84852 * units.m).magnitude == pytest.approx(186.95) | ||
| assert thermolib.isa_temperature(99999 * units.m).magnitude == pytest.approx(186.95) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this the same value pytest.approx(186.95) as the line before? Seeing that other checks show this pattern too. it looks like a range which gives the same result.
Is that 99999 a 99.999 with fractions? (I can't enter that in the UI)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conversion routine works from 0 to 99999m/99.999km (effectively for any number < 100km).
I changed the order of the tests to make this more apparent.
The SideView goes only to 99km as we add internally 1km to the selected top height to define the grid.
But this can be improved upon. I just changed the code to allow values up to 99.9km and changed the precision to be dependent on the type, ie for pressure_altitude, a single digit is allowed. 4 digits is only allowed for pressure, where this is necessary for the large altitudes.
I also adapted the code to allow smaller pressures, as previously above 99km the pressure could not be converted back to km.
|
Setting the largest value and toggling through the vertical axis options shows a traceback. traceback.mp4 |
ReimarBauer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
besides the linter failure I pushed last version, it looks now good, see PR #2961
Extend range of SideView to 99km.
Purpose of PR?:
Fixes #2935
Does this PR introduce a breaking change?
no
Additional information for reviewer? :
See also #2930
Checklist:
<type>: <subject>