Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve RealPointCollection Design #36

Open
awalter17 opened this issue May 3, 2018 · 1 comment
Open

Improve RealPointCollection Design #36

awalter17 opened this issue May 3, 2018 · 1 comment

Comments

@awalter17
Copy link
Contributor

Hi!

This issue outlines several possible improvements to the RealPointCollection design, originally noted by @ctrueden.

  1. Preserve point order.
    DefaultWritableRealPointCollection does not preserve point order. Meaning, someone could create a RealPointCollection with a Collection of points and then retrieve an Iterable via points() with the points in a different order. Ideally, point order should be preserved.

  2. Avoid small objects.
    DefaultWritableRealPointCollection is backed by a HashMap which hashes on Trove double lists, meaning there are many small objects. The purpose of this was to ensure that the hash is on point location, not object reference. It would be nice to hash on something else, possibly something like XOR of the point coordinates? XOR can't be used because point pairs like (1, 3) and (3, 1) would have the same hash.

  3. Naive implementation of RealPointCollection.
    All the current implementations of RealPointCollection are backed by a secondary data structure, which makes test(...) very efficient. However, it could be useful to have a "lighter-weight" RealPointCollection that isn't backed by an additional data structure for times when test(...) performance isn't critical (i.e. SciView).

  4. Generalize DefaultWritableRealPointCollection constructor.
    DefaultWritableRealPointCollection#DefaultWritableRealPointCollection(java.util.Collection) takes a Collection<L> as input which is limiting. Instead this constructor should be generalized to take Collection<? extends L>.

  5. Improve performance of addPoint(...) bounds update.
    When calling addPoint(...) on DefaultWritableRealPointCollection, the bounds are updated by looping all points in the collection. However, this is very inefficient. The bounds can be updated by simply checking if the new point is smaller/larger than the current min/max.

Please let me know your thoughts on these suggested improvements. Also note, there have been some suggested improvements to RealPointCollection in #34 as well.

@ctrueden
Copy link
Member

ctrueden commented May 6, 2018

Thanks @awalter17. Note that (5) is solved by PR #35.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants