Dat #31
Dat #31
Conversation
|
A couple of general comments: do not parenthesize conditions; also errors should be covered by the tests. |
|
Sorry @prakharcode for not raising this issue beforehand but a straight port from C won't cut it here. Most of the things that The best approach would thus be to replace the call to |
Pull Request Test Coverage Report for Build 188
💛 - Coveralls |
src/constants.jl
Outdated
| @@ -42,4 +43,40 @@ const ELB = 1.550519768e-8 | |||
| const JD_MIN = -68569.5 | |||
| const JD_MAX = 1e9 | |||
|
|
|||
| export DRIFT, LS_1972 | |||
There was a problem hiding this comment.
These constants are specific to the LeapSeconds module and thus should also live there.
There was a problem hiding this comment.
They also should not be exported because they are not generally useful.
src/LeapSeconds.jl
Outdated
| @@ -3,9 +3,31 @@ module LeapSeconds | |||
| using ERFA | |||
| using OptionalData | |||
| using RemoteFiles | |||
| include("Periods.jl") | |||
There was a problem hiding this comment.
Including the same file in multiple places is an antipattern and a clear indication that you are increasing the coupling of the code and fighting against its structure, i.e. you are doing something wrong. See my comment below about where to put the constants.
The LeapSeconds module uses no functionality of the Periods module and therefore should not import it.
src/LeapSeconds.jl
Outdated
| export leapseconds, LSK, LSK_FILE, LSK_DATA, fractionofday | ||
|
|
||
|
|
||
| struct changes |
There was a problem hiding this comment.
This whole structure seems unnecessary to me.
src/LeapSeconds.jl
Outdated
| changes(t,leapseconds,drift) | ||
| end | ||
|
|
||
| CHANGE = changes(LS_1972, DRIFT) |
There was a problem hiding this comment.
It does not really make sense to define a function and then only call it once from the top (module) level. If it is top-level code that only runs on import then just put it on the module level. I do not think this is needed at all though.
src/Periods.jl
Outdated
| @@ -1,7 +1,6 @@ | |||
| module Periods | |||
|
|
|||
| import Base: *, /, get, isapprox, show | |||
|
|
|||
src/conversions.jl
Outdated
| @@ -561,7 +562,7 @@ end | |||
|
|
|||
| end | |||
|
|
|||
| function cal2jd(iy, im, id) | |||
| @inline function cal2jd(iy, im, id) | |||
There was a problem hiding this comment.
Please do not slap @inline on every function. This function for example is far too large to be force-inlined. See more information here https://en.wikipedia.org/wiki/Inline_expansion
src/constants.jl
Outdated
| ( 39126.0, 0.0025920 ), | ||
| ( 39126.0, 0.0025920 )] | ||
|
|
||
| LS_1972 = [ |
There was a problem hiding this comment.
Use the same pattern here as I have outlined above for drift.
While it is nice that the orginal code used human-readable dates, i.e. Gregorian calendar dates, from a maintenance perspective, it is more efficient to use Julian Day numbers directly and we will not edit this code a lot in the future. The leap seconds has replaced this scheme of keeping UTC in sync with the rotation of the Earth after all.
src/constants.jl
Outdated
| @@ -42,4 +43,40 @@ const ELB = 1.550519768e-8 | |||
| const JD_MIN = -68569.5 | |||
| const JD_MAX = 1e9 | |||
|
|
|||
| export DRIFT, LS_1972 | |||
|
|
|||
| const DRIFT = [ | |||
There was a problem hiding this comment.
While I know that this is the structure from the C code, I do not think that it makes sense to adopt it here. You are doing a lot of work to get the data into a consumable state above. Why not have it in a usable form directly?
The formula for drift is drift = days_since_ref_epoch * drift_rate or drift = (jd - ref_epoch) * drift_rate. I would propose two have two tuples: DRIFT_EPOCHS and DRIFT_RATES. We also do not need to duplicate values like the C code.
# Use unmodified Julian Day number since this will also be the input
const DRIFT_EPOCHS = (
2.4373005e6,
2.4376655e6,
2.4387615e6,
2.4391265e6,
)
const DRIFT_RATES = (
0.0012960,
0.0011232,
0.0012960,
0.0025920,
)We can then calculate the drift like this:
idx = findlast(jd .>= DRIFT_EPOCHS)
drift = (jd - DRIFT_EPOCHS[idx]) * DRIFT_RATES[idx]Far simpler, isn't it?
Codecov Report
@@ Coverage Diff @@
## master #31 +/- ##
==========================================
+ Coverage 87.4% 87.53% +0.12%
==========================================
Files 7 7
Lines 389 393 +4
==========================================
+ Hits 340 344 +4
Misses 49 49
Continue to review full report at Codecov.
|
|
I will merge this as it is and propose a few improvements in a new PR. Thanks @prakharcode 👍 |
|
Thank you 😄 |
Ported dat into leapseconds