igr / julian-day

Precise Julian Day Java library
BSD 2-Clause "Simplified" License
4 stars 0 forks source link

Rounding error regression in conversion to double #4

Closed cusher closed 5 months ago

cusher commented 5 months ago

We have a time conversion class Time with a handful of methods for converting Instant to/from other formats we use, including MJD. I'll include the relevant parts here.

With jodd.time.JulianDate:

    public static Instant fromMJD(double mjd) {
        return JulianDate.fromModifiedJulianDate(mjd).toInstant();
    }

    public static double toMJD(Instant instant) {
        return JulianDate.of(instant).getModifiedJulianDate().toDouble();
    }

I updated this to test using jodd.julianday.JulianDay in it's place with:

    public static Instant fromMJD(double mjd) {
        return Instant.ofEpochMilli(JulianDay.of(mjd).add(MODIFIED_JULIAN_DAY_0).toUnixMilliseconds());
    }

    public static double toMJD(Instant instant) {
        return JulianDay.ofUnixMilliseconds(instant.toEpochMilli()).valueAsModifiedJulianDay().toDouble();
    }

A bit less terse, but that's not an issue.

The following test, which used to pass, now fails:

    @Test
    public void Time_convertsToMjd() {
        assertThat(Time.toMJD(Instant.parse(TIME_STRING)), closeTo(57447.932199074, 1.0e-10));

        assertThat(Time.toMJD(Instant.parse("2016-12-31T21:36:00Z")), is(57753.9));
        assertThat(Time.toMJD(Instant.parse("2017-01-01T00:00:00Z")), is(57754.0));
        assertThat(Time.toMJD(Instant.parse("2017-01-01T02:24:00Z")), is(57754.1));
        assertThat(Time.toMJD(Instant.parse("2017-01-01T04:48:00Z")), is(57754.2));
        assertThat(Time.toMJD(Instant.parse("2017-01-01T07:12:00Z")), is(57754.3));
    }

The first assertion passes, as does the 57754.0 check, but the rest are off by a small rounding error.

Don't have much time to look further into this at the moment, but passing it along at least.

igr commented 5 months ago

I will take a look!

Btw, I can add more utility methods to make it less terse and will do so!

igr commented 5 months ago

@cusher new methods:

    public static Instant fromMJD(final double mjd) {
        return JulianDay.ofModifiedJulianDay(mjd).toInstant();
    }

    public static double toMJD(final Instant instant) {
        return JulianDay.ofInstant(instant).valueAsModifiedJulianDay().toDouble();
    }
igr commented 5 months ago

Releasing 7.2.0 atm

cusher commented 4 months ago

Looks good, thanks!