correction for altitudes based on takeoff altitude#161
correction for altitudes based on takeoff altitude#161zweefan wants to merge 1 commit intoGliderGeek:mainfrom
Conversation
GliderGeek
left a comment
GliderGeek
left a comment
There was a problem hiding this comment.
nice addition. hereby some pointer for (minor) improvements. also: could you add an entry in the CHANGES.md file?
|
|
||
|
|
||
| def run(url, source, download_progress=None, analysis_progress=None, on_success=None, on_failure=None): | ||
| def run(url, source, to_elevation, download_progress=None, analysis_progress=None, on_success=None, on_failure=None): |
There was a problem hiding this comment.
i think it helps to explicitly show the optional behavior of to_elevation:
| def run(url, source, to_elevation, download_progress=None, analysis_progress=None, on_success=None, on_failure=None): | |
| def run(url, source, to_elevation=None, download_progress=None, analysis_progress=None, on_success=None, on_failure=None): |
| args = (url, get_url_source(url), self.set_download_status, self.set_analyse_status, | ||
| try: | ||
| to_elevation = int(self.to_elevation_input.GetValue()) | ||
| except: |
There was a problem hiding this comment.
When using a bare except clause, this also catches a KeyboardInterrupt or other signal.
https://docs.python.org/3/library/exceptions.html#exception-hierarchy
i prefer to only catch the instances of Exception
| except: | |
| except Exception: |
| self.set_performance_entry("t_finish", "Finish time", "text", "neutral", "[local time]", True, False, True, True) | ||
| self.set_performance_entry("h_start", "Start height", "number", "high", "[m]", True, True, True, True) | ||
| self.set_performance_entry("h_finish", "Finish height", "number", "high", "[m]", True, False, True, False) | ||
| self.set_performance_entry("h_finish", "Finish height", "number", "low", "[m]", True, False, True, True) |
There was a problem hiding this comment.
is it really umambiguous that a lower finish height is always better? i guess it makes sense for the last leg, but for others along the task this really depends? (for instance if you round a turnpoint downwind).
i think the original code was also wrong, so maybe "neutral" would be best here?
now that i think of this, the same goes for the start height...
| def init_all(self, trip, gps_altitude): | ||
| start_h = trip.fixes[0]['gps_alt'] if gps_altitude else trip.fixes[0]['pressure_alt'] | ||
| start_t = trip.refined_start_time | ||
| def get_height(self, trip_index): |
| def get_height(self, trip_index): | ||
| if (self.gps_altitude): | ||
| # return gps altitude corrected for start elevation | ||
| return self.trip.fixes[trip_index]['gps_alt'] - self.elevation_correction |
There was a problem hiding this comment.
i think it is clearer if the elevation correction is simply added here and that the value can be both negative and positive. if this is changed, the calculation should be changed too. also there might be other places where this is used.
|
|
||
| to_elevation_sizer = wx.BoxSizer(wx.HORIZONTAL) | ||
|
|
||
| text = wx.StaticText(panel, label="Field elevation:") |
There was a problem hiding this comment.
Maybe nice to add a unit:
is this above MSL?
| text = wx.StaticText(panel, label="Field elevation:") | |
| text = wx.StaticText(panel, label="Field elevation [m]:") |
3 participants
No description provided.