Refactor _SGDB4 class #171
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
==========================================
- Coverage 88.45% 87.81% -0.65%
==========================================
Files 16 16
Lines 2391 2626 +235
==========================================
+ Hits 2115 2306 +191
- Misses 276 320 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
The refactoring of the I will go through the code once or twice early next week, but feel free to give suggestions. |
Oh yeah, well spotted. Removed and pre-commit checks pass. |
pyorbital/orbital.py
Outdated
| params = {"utc_time": utc_time} | ||
| params["ts"] = self._get_timedelta_in_minutes(params) |
There was a problem hiding this comment.
why the switch to dict? for the rest we’re using objects, right? So how about a class for keplerian computations?
There was a problem hiding this comment.
I didn't want to put all the shared intermediate variables to attributes, which are already pretty numerous. There are also so many interconnections that I thought this was at least the easiest way to sort them out somehow.
I'll have another look tomorrow from the "add a new class" perspective.
There was a problem hiding this comment.
Ok, after "a bit" of shuffling, I did 6fb6efd
pyorbital/orbital.py
Outdated
| def _calculate_mean_motion_and_semi_major_axis(self): | ||
| a_1 = (XKE / self.mean_motion) ** (2.0 / 3) | ||
| delta_1 = ((3 / 2.0) * (CK2 / a_1**2) * ((3 * np.cos(self.inclination)**2 - 1) / | ||
| (1 - self.excentricity**2)**(2.0 / 3))) | ||
| a_0 = a_1 * (1 - delta_1 / 3 - delta_1**2 - (134.0 / 81) * delta_1**3) | ||
| delta_0 = ((3 / 2.0) * (CK2 / a_0**2) * ((3 * np.cos(self.inclination)**2 - 1) / | ||
| (1 - self.excentricity**2)**(2.0 / 3))) | ||
|
|
||
| self.original_mean_motion = self.mean_motion / (1 + delta_0) | ||
| self.semi_major_axis = a_0 / (1 - delta_0) |
There was a problem hiding this comment.
this method has minimal side effects, could we remove them and just return the mean motion and semi major axis?
There was a problem hiding this comment.
Do you mean that instead of calculating them together we'd have the same intermediate computations done twice and then have two methods? I think I considered that and opted for the merged one for less computations. I don't mind changing this to two methods though.
There was a problem hiding this comment.
no calculating them together is fine, just that I thought it would be nicer to have eg self.original_mean_motion, self.semi_major_axis = self._calculate_mean_motion_and_semi_major_axis()
There was a problem hiding this comment.
IMO it looks better, thanks a lot!
This PR refactors the _SGDB4 class.
__init__()propagate()flake8 pyorbital