jd2cal #23
jd2cal #23
Conversation
src/conversions.jl
Outdated
| f += 1.0 | ||
| end | ||
| d = round(date-f1) + round(date1-f2) + round(f1+f2-f) | ||
| jd = round(d) + 1. |
There was a problem hiding this comment.
You have to be careful with your number types. jd for example is a long in the C code, i.e. an integer. Many of the lines below use integer arithmetic in the C version including integer (truncating) division whereas your version uses floating point math which will get different results.
jd = Int(round(d) + 1)
src/conversions.jl
Outdated
| d = round(date-f1) + round(date1-f2) + round(f1+f2-f) | ||
| jd = round(d) + 1. | ||
|
|
||
| l = jd + 68569. |
There was a problem hiding this comment.
Every long literal in C, e.g. 68569L, should be an integer literal in Julia.
src/conversions.jl
Outdated
| i = (4000. * (l + 1.)) / 1461001. | ||
| l -= (1461. * i) / 4. - 31. | ||
| k = (80. * l) / 2447. | ||
| id = Int(floor((l - (2447. * k) / 80.))) |
There was a problem hiding this comment.
This is casting from long to int in the C code which means you do not need it here since all parameters should already be ints.
src/conversions.jl
Outdated
| jd = round(d) + 1. | ||
|
|
||
| l = jd + 68569. | ||
| n = (4. * l) / 146097. |
There was a problem hiding this comment.
You need to explicitly use integer division on the Julia side, e.g. n = (4 * l) ÷ 146097 (÷ tab-completes from \div in editors with Julia plugins) or n = div((4 * l), 146097). I would prefer the unicode operator.
src/conversions.jl
Outdated
| @inline function jd2cal(jd1, jd2) | ||
| dj = jd1 + jd2 | ||
| if dj < JD_MIN || dj > JD_MAX | ||
| error("Date is not acceptable") |
There was a problem hiding this comment.
You should use plain error only in scripts. Use more explicit exception types in library code. Here throw(ArgumentError("...")) would be appropriate.
I would also like a more descriptive error message, e.g. "Julian date is outside of the representable range." or something like that.
* Added test for new dtdb routine. Fix comment.
... and minor cleanups
Codecov Report
@@ Coverage Diff @@
## master #23 +/- ##
===========================================
- Coverage 97.6% 86.42% -11.18%
===========================================
Files 6 7 +1
Lines 292 361 +69
===========================================
+ Hits 285 312 +27
- Misses 7 49 +42
Continue to review full report at Codecov.
|
Pull Request Test Coverage Report for Build 173
💛 - Coveralls |
@helgee the type conversion is proving to be a problem. I've tried with
roundstill the date part gives an error. @giordano any solution to this?