Fix IncompatibleArgumentsError on Moon's observation events

[rhannequin]

On some dates, Moon#observation_events would throw a IncompatibleArgumentsError.

This was due to the interpolation method. It takes 3 or 5 approximately equidistant values and interpolate a value between them. Because angles are normalized, an angle that would be 365° would be normalized into 5°. While it makes sense in the library, it breaks the suite of numbers when using interpolation. A list that would initially and logically be [345, 355, 365] is changed into [345, 355, 5], which makes the interpolation logic wrong.

This fixes this issue by denormalizing values for interpolation when it seems it is needed. When we have consecutive values that are suddenly very far from each other, the method will fix them so that the suite stays uniform.

To be honest, this seems a bit hacky and I have no way of making sure this actually works fine without adding tons and tons of new examples with different configurations and making sure the results still make sense. But the library is already quite well test-covered and everything is green, so I guess this new method is fine.

Fixes #106
Fixes #112

[trevorturk]

I think this all makes sense.

I see your point about not wanting to add tons of test data etc. I think I may be able to help. I'm planning to give Astronoby another shot (I have an adapter system so I can easily swap between SunCalc etc) and I'll add some code to report back if I'm finding this error again, or if the difference between different sources are extreme. I think this will help us have a lot of locations/times passing through the library, but only reporting back for edge cases so it won't be overwhelming.

It may be a few weeks (holidays now in the US, so no school for the kids!) but I'll report back ASAP when I have the system set up with reporting so I can share anything that seems suspicious. I don't know if you want to merge this in, or I can work off the branch. No worries either way. Thank you!

[rhannequin]

Thanks a lot for helping with testing the library in real conditions 🙏

I made an experiment documented in #112 and I'm not satisfied with the overall error rate. I would like to drop down to less than 1% error rate and then publish a new version of the gem. As I described in the issue, the current error rate is estimated to 5%, which is not acceptable.

I have a strong feeling that a lot of the errors are still happening because of interpolation problems. I'm going to investigate further before merging this PR.

[trevorturk]

Ah, that's a great idea. Perhaps worth running on the full globe if you get the error rate down low enough. Also I wonder if some weird errors might be symmetrical but who knows.

I was thinking perhaps we could do an issue report template that would expect to have a failing test case already made in the PR so it'd be super easy to run with it. I don't know that I've seen this elsewhere, but I do it for my own app, where I have some "edge case tests" etc that exercise weird stuff I've seen in production, like regression testing.

[rhannequin]

Update:

The updated version of this PR's commit includes fixes that greatly improve the overall error rate.

I've run the following script to cover many many cases:

tries = 0
interpolation_errors = []
math_errors = []
argument_errors = []
estimated_tries = 365 * 179 * 359
(0..364).to_a.each do |i|
  (-89..89).to_a.each do |latitude|
    (-179..179).to_a.each do |longitude|
      begin
        tries += 1
        pp "Try #{tries}/#{estimated_tries}"
        date = Date.new(2024, 1, 1).next_day(i)
        observer = Astronoby::Observer.new(
          latitude: Astronoby::Angle.from_degrees(latitude),
          longitude: Astronoby::Angle.from_degrees(longitude)
        )
        moon = Astronoby::Moon.new(time: date)
        observation_events = moon.observation_events(observer: observer)
      rescue Astronoby::IncompatibleArgumentsError
      interpolation_errors << [i, latitude, longitude]
      rescue Math::DomainError
        math_errors << [i, latitude, longitude]
      rescue ArgumentError
        argument_errors << [i, latitude, longitude]
      end
    end
  end
end

The results are: 0 errors!!
That's why I'll consider #112 fixed as well when merging this commit.

As a side note, running these 23,455,265 combinations took several hours on my machine. It could be worth it at some point to investigate possible performance improvements.

[trevorturk]

🎉

[trevorturk]

This would likely be another issue, but I had a thought that you might want to take a pass comparing Astronoby to SunCalc, not that it's likely to be more accurate, but I do wonder if you checked that timestamps were less than 50 min apart or something along those lines, it could expose timezone issues or perhaps other interesting things. It could serve to be a point of marketing Astronoby if you can claim better accuracy against alternative libraries.

[rhannequin]

@trevorturk Thank you for the suggestion!

In that case it would make sense to have an "absolute truth" source, which would be the IMCCE in my opinion (extremely accurate and easy to query). It would serve as a source of truth for Astronoby's results and a source of comparison with SunCalc.

In your opinion, what would be the best strategy to find the right balance between coverage and efficiency?

• not enough coverage (number and variety of tests) could reduce relevance
• coverage too large could make the test suite too long and discourage from some testing practices, or CI would be too long

My fear is also to discover how wrong the library could be in some cases but at least that's a good motivation to work towards.

[trevorturk]

I was thinking of just trying to use your existing test script from above to check and see how many times you have that are totally different from SunCalc. Then you could check against the source of truth (IMCCE) for a few to see which library has it wrong. If it's SunCalc, oh well, now we know. If it's Astronoby, perhaps it's some time zone conversion issue or something that happens en mass and it'd be nice to fix, just as you've fixed the errors in this PR.

Anyway, just an idea to try to run scripts to "sanity check" some of the results. Also, I'll still plan to compare in real world usage, and I can compare against somewhat official data sources with the various weather APIs.

[rhannequin]

That's a great idea. I've created a dedicated issue #113 to take care of this.