NASA-AMMOS / aerie

A software framework for modeling spacecraft.
https://nasa-ammos.github.io/aerie-docs/
MIT License
73 stars 19 forks source link

Performance/intervalmap #1371

Closed bradNASA closed 7 months ago

bradNASA commented 8 months ago

Description

IntervalMap changed to use TreeSet instead of ArrayList for Segments to speed up set(), which is used to construct an IntervalMap from a list of Segments. Some SPHEREx scheduling code for merging SimulationResults is very slow because set() makes IntervalMap construction O(n^2). The use of TreeSet makes that O(n log n). I am not sure what other code may benefit from this.

Joel Courtney pointed out that the use of a list is more efficient in some cases, for example, for appending non-overlapping Segments. He thought it might make sense to keep the list and use a tree in some cases. There may be some better data structure that takes advantage of both, like an ordered list with binary search operations; maybe a skip list would, too.

There's a shortcut checking whether the input segments meet invariant conditions in order to ingest Segments without using set(), which handles overlapping.

There are changes to IntervalAlgebra for faster low-level interval queries.

Verification

Unit tests pass seemingly faster (maybe 3.25 instead of 3.75 minutess), but variance is high. I still need to verify it solves the problem in the SPHEREx model.

Documentation

None, afaik

Future work

Interval code elsewhere in Aerie may also need updating like this.

bradNASA commented 8 months ago

@mattdailis Any reason we shouldn't merge this?

bradNASA commented 8 months ago

I haven't looked at every change to IntervalMap in detail, but I do know that Pranav and I tested the bejeezus out of that class back in the day. It sounds theoretically sound and the tests pass. 👍

Yeah, I was impressed with the detail of the test suite.

dandelany commented 7 months ago

Thanks @bradNASA , merging!