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

Implementation of Table Index Engine using sortedcontainers #7531

Closed
wants to merge 0 commits into from
Closed

Implementation of Table Index Engine using sortedcontainers #7531

wants to merge 0 commits into from

Conversation

grantjenks
Copy link
Contributor

This is an initial implementation of a table index engine using sortedcontainers. It does not pass all tests. It's entirely based on the FastRBT engine in bst.py. My thought for moving forward would be to introduce a new engine like SortedArray and then deprecate the old engines based on bintrees.

Using the bintrees design, I replaced bintrees.FastRBTree with sortedcontainers.SortedDict. The flaw here is that SortedDict requires that keys be hashable and the Time object doesn't support hashing though it supports comparisons. Would it be too difficult to add hashing to Time objects? Is that an unreasonable requirement for index table values?

I'm also curious to better understand why values added to an index are sometimes converted to tuples and sometimes not. Why does bst.py::FastBase.__init__ convert keys to tuples but bst.py::FastBase.add does not?

Reference: Issue #6539.

@astropy-bot
Copy link

astropy-bot bot commented Jun 4, 2018

Hi there @grantjenks 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I noticed the following issue with this pull request:

  • Changelog entry not present (or pull request number missing) and neither the Affects-dev nor the no-changelog-entry-needed label are set

Would it be possible to fix this? Thanks!

If there are any issues with this message, please report them here.

@bsipocz bsipocz requested a review from taldcroft June 4, 2018 22:23
@bsipocz bsipocz added this to the v3.1 milestone Jun 4, 2018
@bsipocz bsipocz added the table label Jun 4, 2018
@taldcroft
Copy link
Member

@grantjenks -

Yes, in theory Time should be hashable. There may be some subtleties that require some thinking, but here is a stub implementation that will be good enough for testing and debugging:

diff --git a/astropy/time/core.py b/astropy/time/core.py
index c99f913..ee7571d 100644
--- a/astropy/time/core.py
+++ b/astropy/time/core.py
@@ -789,6 +789,11 @@ class Time(ShapedLikeNDArray):
         self._time.jd1[item] = value._time.jd1
         self._time.jd2[item] = value._time.jd2
 
+    def __hash__(self):
+        if self.ndim != 0:
+            raise TypeError("unhashable type: '{}'".format(self.__class__.__name__))
+        return hash((self.jd1, self.jd2, self.scale))
+
     def light_travel_time(self, skycoord, kind='barycentric', location=None, ephemeris=None):
         """Light travel time correction to the barycentre or heliocentre.

With this in place I saw two classes of errors still, one that is happening in Table.remove_row() and one that is happening in sortedcontainers.sortedlist. I didn't have a chance to dig any deeper.

See #7532.

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

Successfully merging this pull request may close these issues.

None yet

3 participants