DM-7199: afwTable's .getX()/.getY() do not appear in dir() #238

mrawls · 2017-05-25T23:24:41Z

No description provided.

parejkoj

Should we have a unittest for this? I'm not sure at what level to put it (since this is the base class for so many things), but it seems like this could be a bit fragile (I don't have any evidence for that, just vague feelings)?

Also, has anyone tried benchmarking it for one of our classes a ways down the hierarchy? set is pretty fast, but I wonder if we have anything with enough __bases__ to matter?

parejkoj · 2017-05-26T05:18:37Z

python/lsst/afw/table/base/baseContinued.py

@@ -283,6 +283,24 @@ def asAstropy(self, cls=None, copy=False, unviewable="copy"):
            )
        return cls(columns, meta=meta, copy=False)

+    def __dir__(self):
+        # This custom dir is necessary due to the custom getattr below.


Should this be a docstring instead of a comment?

parejkoj · 2017-05-26T05:27:15Z

python/lsst/afw/table/base/baseContinued.py

+                    result |= recursive_get_class_dir(subcls)
+            result |= set(cls.__dict__.keys())
+            return result
+        return sorted(set(dir(self.columns)) | set(dir(self.table)) |


I didn't know about the set operators (pipe for union, in this case). That's cool.

Separately, I think this return should be separated from the above by a line, since it's not part of that inner function.

parejkoj · 2017-05-26T05:27:43Z

python/lsst/afw/table/base/baseContinued.py

+            result |= set(cls.__dict__.keys())
+            return result
+        return sorted(set(dir(self.columns)) | set(dir(self.table)) |
+                      recursive_get_class_dir(type(self)) | set(self.__dict__.keys()))


Can't you just do set(self.__dict__) at the end?

Indeed so, at least in python 3. I'm not sure if this would work in python 2 though?

I think you're right. Burned by py2 again.

parejkoj · 2017-05-26T05:55:24Z

python/lsst/afw/table/base/baseContinued.py

+            '''
+            result = set()
+            if cls.__bases__:
+                for subcls in cls.__bases__:


I guess this works, but the use of __bases__ feels exceptionally clever.

Does the MRO matter here at all? Probably not, since it's all going into a set.

the... MRO?

@mrawls https://utcc.utoronto.ca/~cks/space/blog/python/MethodResolutionOrder

Ah. I don't think this matters, because it's all going into a set, and ultimately lands in a sorted list that excludes duplicates. And I choose to take "exceptionally clever" as a compliment, largely on Russell's behalf.

Note that you are often much more clever writing code than trying to understand it later. That was the nature of my comment.

mrawls · 2017-05-26T19:52:26Z

I'm willing to write a unittest, but I also have no idea where it should live, or even what exactly it would test. Suggestions welcome.
I have not tried timing anything that has a bazillion subclasses to see if it slows things down. Unless I'm mistaken, though, the common use case for calling dir is in an interactive environment, not when a command line task is run; I can't imagine it taking a frustrating amount of time, e.g., anywhere near as long as it does to import afw in the first place.
Also tagging @r-owen in case he has insights (we worked together on this fix).

mrawls · 2017-05-26T19:56:59Z

Meant to also say - Jenkins passed for py2, but not for py3, because of test_obs.py? I will need to address this, it seems, hopefully with help from @r-owen :) https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py3/24053/console#console-section-3

parejkoj · 2017-05-26T20:04:57Z

Re-run jenkins. That looks like one of the sporadic failures we've been getting.

mrawls · 2017-05-26T23:21:28Z

Oh good, Jenkins passed this time!
Unless you have a specific suggestion for a test that would make sense to go along with this change and/or a benchmark I should run, I'd like to go ahead and wrap this up.

r-owen · 2017-05-27T15:39:56Z

I agree that a unit test is a good idea. I suggest using the test script you already have as the basis for a new test method in an existing test of SourceCatalog. Something like this:

def testDir(self):
    ...make a source catalog named "cat" or "srcCat" with a minimal schema
    attrNames = dir(cat)
    desiredNames = set("_columns", "getX", ...)  # at least one from each class and subclass
    self.assertTrue(desiredNames.issubset(attrNames))  # I could not find a more specific assert in unittest, but I hope I overlooked something

Also, I would be tempted to move the special function definition up to the module level, to make sure it only gets declared once (I am not sure if Python is smart enough to realize that nothing in the function definition depends on variables local to the __dir__ method, and thus can cache the definition).

As to cleverness: the fact that Catalog overrides __getattr__ to delegate unrecognized methods makes it hard to show what methods are available. I hope someday we can find a cleaner way to delegate that does not require overriding __getattr__, at which point we can stop overriding __dir__.

mrawls · 2017-05-28T21:01:07Z

OK, that makes sense, I'm happy to add this test.

When you say "move the special function definition up to the module level," do you mean just the custom __dir__ or the magic __getattr__ too? Right now they live in afw/table/base; do you mean move one or both of these override functions into a standalone script to apply to all of afw?

I'm headed off on vacation for a week tonight and won't be able to get to this until I'm back. If you want to finish it without me, @r-owen, you are welcome to, but you certainly don't have to.

r-owen · 2017-05-30T18:09:27Z

The function I think might want to be moved is recursive_get_class_dir; right now it is defined inside of __dir__ and I lean towards defining it in baseContinued.py (as a function visible everywhere in that file) merely to make sure it is only defined once. It probably doesn't matter, and I would not suggest making it visible outside that file (if such a function is ever wanted in more than one place then I'd move it to utils). It probably doesn't matter enough to bother.

mrawls · 2017-06-07T20:57:03Z

I think this should be ready now. It turns out I ran Jenkins before the new test was actually doing anything, but it passes when I run it locally with pytest. I can run Jenkins again if you think it's necessary.
Additionally, if you or @r-owen have a different subset of attributes you'd like to see in desiredNames I'm happy to switch it up, but I think I got something from each class and subclass.

parejkoj

See further comments.

The thing I was worried about re:speed was exactly in an interactive environment: ipython/jupyter use dir() to populate the auto-completion list. Might be worth copying some of your test code into a jupyter notebook, and typing catalog.[TAB] and see if the list pops up quickly.

parejkoj · 2017-06-07T22:15:36Z

python/lsst/afw/table/base/baseContinued.py

+            '''
+            result = set()
+            if cls.__bases__:
+                for subcls in cls.__bases__:


Note that you are often much more clever writing code than trying to understand it later. That was the nature of my comment.

parejkoj · 2017-06-07T22:15:54Z

python/lsst/afw/table/base/baseContinued.py

+            result |= set(cls.__dict__.keys())
+            return result
+        return sorted(set(dir(self.columns)) | set(dir(self.table)) |
+                      recursive_get_class_dir(type(self)) | set(self.__dict__.keys()))


I think you're right. Burned by py2 again.

parejkoj · 2017-06-07T22:16:43Z

tests/testDir.py

+        attrNames = dir(catalog)
+        desiredNames = set(['_columns', 'getX', 'asAstropy', 'hasPsfFluxSlot'])
+        self.assertTrue(desiredNames.issubset(attrNames))
+


Should have the MemoryTestCase here.

parejkoj · 2017-06-07T22:20:56Z

tests/testDir.py

+        # Compare catalog attributes with those from various catalog subclasses
+        attrNames = dir(catalog)
+        desiredNames = set(['_columns', 'getX', 'asAstropy', 'hasPsfFluxSlot'])
+        self.assertTrue(desiredNames.issubset(attrNames))


This seems like a reasonable way to test that dir(catalog) contains those things. I'd suggest we also include getY in the list. Anything else we should also include? It doesn't hurt to be comprehensive in this test.

mrawls · 2017-06-07T23:36:01Z

IPython test (my jupyter notebook install is giving me python version trouble at present) -> catalog.[TAB] pops up a full list nice and quickly as I would expect 👍
Added getY to the list. There are 160 total attributes at present(!). Happy to add more but I don't have any particular favorites...
Added all the test boilerplate that should have been there before (thanks for DMing me the link)

parejkoj reviewed May 26, 2017

View reviewed changes

mrawls force-pushed the tickets/DM-7199 branch from 5b95ae6 to fb15e2b Compare May 26, 2017 23:20

mrawls force-pushed the tickets/DM-7199 branch from eda3b5b to e426429 Compare June 7, 2017 02:31

Add custom __dir__ to return all methods available in afwTable

0060a1b

mrawls force-pushed the tickets/DM-7199 branch 2 times, most recently from bf2cb27 to 6505879 Compare June 7, 2017 20:50

parejkoj reviewed Jun 7, 2017

View reviewed changes

mrawls force-pushed the tickets/DM-7199 branch from b2131a1 to 461e7c3 Compare June 7, 2017 23:57

Add a test for the custom afwTable __dir__

f16258e

mrawls force-pushed the tickets/DM-7199 branch from 5388aae to f16258e Compare June 8, 2017 00:02

mrawls merged commit f16258e into master Jun 8, 2017

ktlim deleted the tickets/DM-7199 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-7199: afwTable's .getX()/.getY() do not appear in dir() #238

DM-7199: afwTable's .getX()/.getY() do not appear in dir() #238

mrawls commented May 25, 2017

parejkoj left a comment

parejkoj May 26, 2017

parejkoj May 26, 2017

parejkoj May 26, 2017

mrawls May 26, 2017

parejkoj Jun 7, 2017

parejkoj May 26, 2017

mrawls May 26, 2017

kfindeisen May 26, 2017

mrawls May 26, 2017

parejkoj Jun 7, 2017

mrawls commented May 26, 2017

mrawls commented May 26, 2017

parejkoj commented May 26, 2017

mrawls commented May 26, 2017

r-owen commented May 27, 2017

mrawls commented May 28, 2017

r-owen commented May 30, 2017

mrawls commented Jun 7, 2017

parejkoj left a comment

parejkoj Jun 7, 2017

parejkoj Jun 7, 2017

parejkoj Jun 7, 2017

parejkoj Jun 7, 2017

mrawls commented Jun 7, 2017

DM-7199: afwTable's .getX()/.getY() do not appear in dir() #238

DM-7199: afwTable's .getX()/.getY() do not appear in dir() #238

Conversation

mrawls commented May 25, 2017

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrawls commented May 26, 2017

mrawls commented May 26, 2017

parejkoj commented May 26, 2017

mrawls commented May 26, 2017

r-owen commented May 27, 2017

mrawls commented May 28, 2017

r-owen commented May 30, 2017

mrawls commented Jun 7, 2017

parejkoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrawls commented Jun 7, 2017