DM-12452: Add FrameDict class #32

r-owen · 2017-10-31T14:51:15Z

No description provided.

from "ind1, ind2" to "from, to"

kfindeisen

Lots of minor feedback, and two serious issues:

FrameDict cannot be used as a FrameSet because the latter's API does not allow for the possibility of illegal frames. I realize that AST (in particular, the relationship between Frame and FrameSet) does not follow the substitution principle anyway, but I'd prefer not to make things worse. Can FrameDict be made a distinct class instead (with conversions to and from FrameSet, if need be)?
Some of FrameDict's mutators are not exception-safe. Fortunately, the problem can be fixed by refactoring _rebuildDict and _addFrameToDict.

I'm also a bit concerned by how stringly-typed the domain system is, but we can cross that bridge when we get to TransformMap. I guess it makes perfect sense in the context of astshim.

kfindeisen · 2017-11-01T20:31:05Z

include/astshim/FrameDict.h

+
+/**
+A convenience wrapper around FrameSet that maintains an internal lookup table ("dictionary")
+of frame domain:index for those contained frames that have non-empty domains.


This statement is specific to the current implementation and is rather longer than a brief description should be. Isn't it cut off in the built documentation?

How about "A FrameSet that can be indexed by frame domain."?

kfindeisen · 2017-11-01T20:46:13Z

include/astshim/FrameDict.h

+- Some AST frame classes have default domain names, e.g. SkyFrame defaults to "SKY". Such default
+names are ignored in order to reduce the chance of accidental collisions.
+- This class is named "FrameDict" instead of "FrameMap", even though C++ lookup tables are usually
+std::map or unordered_map, in order to avoid confusion with AST Mapping.


I don't think this last note is worth mentioning. It's not like you address why it would be called "FrameMap" instead of, say, "NamedFrameSet".

kfindeisen · 2017-11-01T20:57:51Z

include/astshim/FrameDict.h

+    FrameDict &operator=(FrameDict &&) = default;
+
+    /// Return a deep copy of this object.
+    std::shared_ptr<FrameSet> copy() const { return std::static_pointer_cast<FrameDict>(copyPolymorphic()); }


Did you intend to copy-convert to a shared_ptr<FrameSet>?

No. Good catch.

kfindeisen · 2017-11-01T21:14:51Z

include/astshim/FrameDict.h

+    }
+
+    /**
+    Add a Frame connected by a Mapping to an existing initial Frame. A variant of FrameSet::addFrame


Why are you disambiguating this as FrameSet::addFrame instead of addFrame(int, Mapping const &, Frame const &)? It seems strange to refer the user to the base class documentation.

kfindeisen · 2017-11-01T21:57:29Z

include/astshim/FrameSet.h

@@ -205,7 +205,7 @@ class FrameSet : public Frame {
        and the `current` @ref Frame of the `frame` FrameSet. This latter @ref Frame becomes
        the current @ref Frame in this FrameSet.
    */
-    void addFrame(int iframe, Mapping const &map, Frame const &frame) {
+    virtual void addFrame(int iframe, Mapping const &map, Frame const &frame) {


Making a previously un-overridable method overrideable is dangerous, because it adds another way in which the implementation of the parent class can have unexpected effects on the child class. If you want to do this, please:

Remove the call to addFrame from FrameSet(Frame const &, Mapping const &, Frame const &, std::string const &). This will always call FrameSet::addFrame, which may break subclasses that depend on custom functionality in their addFrame. See Effective C++, Item 9 for more details.

Document any self-use of addFrame by other public or protected methods (fortunately, I think the aforementioned constructor is the only one, but please double-check).

Ok, I was definitely in Java mode when I wrote this. I don't see a sensible way to remove the call, and the danger of unexpected side effects from a base class calling its own virtual methods only applies when the call is, in fact, virtual. So assuming the constructor really is the only self-use of addFrame, there's no need to do anything for this item.

Given the fact that it makes you uneasy and Scott Meyers suggests never calling a virtual function from a constructor or destructor, I'm inclined to change this. I added private method FrameSet::_basicAddFrame and call that from the one FrameSet constructor and from FrameSet::addFrame.

kfindeisen · 2017-11-02T17:43:45Z

tests/test_frameDict.py

+        self.checkPersistence(frameDict)
+        self.checkDict(frameDict)
+
+    def testFrameDictFrameSetConstructor(self):


Please try to be consistent about whether your test cases are named test_Something or testSomething. (I can't find anything in the style guide preferring one or the other.)

kfindeisen · 2017-11-02T17:46:12Z

tests/test_frameDict.py

+        indata = np.array([[1.1, 2.1, 3.1], [1.2, 2.2, 3.2]])
+        predictedOut = indata * self.zoom
+        for zoomMap in zoomMapList:
+            assert_allclose(zoomMap.applyForward(indata), predictedOut)


Again, can predictedOut be computed with frameDict?

I prefer the more direct test, as above.

kfindeisen · 2017-11-02T18:00:35Z

tests/test_frameDict.py

+        self.assertEqual(frameDict.getFrame(frameDict.CURRENT).domain, "FRAME2")
+        self.assertEqual(frameDict.getMapping().ident, "zoomMap")
+        self.checkPersistence(frameDict)
+        self.checkDict(frameDict)


Can you add a test for addFrame that tries to use a duplicate domain? Can it check that class invariants are preserved (in particular, all domains are unique, and getIndex can find any applicable frame by its domain)?

kfindeisen · 2017-11-02T18:01:48Z

tests/test_frameDict.py

+        frameDict.setDomain("NEWFRAME2")
+        self.assertEqual(set(frameDict.getAllDomains()), set(["NEWFRAME1", "NEWFRAME2"]))
+        self.assertEqual(frameDict.getIndex("NEWFRAME1"), 1)
+        self.assertEqual(frameDict.getIndex("NEWFRAME2"), 2)


Can you add a test for setDomain that tries to use a duplicate domain? Can it check that class invariants are preserved (in particular, all domains are unique, and getIndex can find any applicable frame by its domain)?

kfindeisen · 2017-11-02T18:02:24Z

tests/test_frameDict.py

+        self.assertEqual(frameDict.getIndex("NEWFRAME1"), 1)
+        self.assertEqual(frameDict.getIndex("NEWFRAME2"), 2)
+
+


Can you add tests for getFrame, getMapping, etc. that try to use a missing domain?

kfindeisen

Looks good. Some comments on remaining issues.

kfindeisen · 2017-11-08T00:19:24Z

include/astshim/FrameDict.h


    /**
-    Add a Frame connected by a Mapping to an existing initial Frame. A variant of FrameSet::addFrame
-    where the initial frame is specified by domain.
+    @copydoc FrameSet::addFrame

    @throws std::invalid_argument if `frame` has a non-empty domain and this FrameDict already
    contains a Frame with that domain


This will cause the throws section to appear after the notes section, but fine.

kfindeisen · 2017-11-08T00:25:40Z

include/astshim/FrameDict.h


    /**
-    Get a list of the domain names for all contained Frames that have non-empty domain names.
+    Variant of @ref addFrame(int, Mapping const &, Frame const &) "addFrame(int, ...)"
+    where the initial frame is specified by domain.


kfindeisen · 2017-11-08T00:35:28Z

include/astshim/FrameDict.h


    using FrameSet::getFrame;

    /**
-    Get a frame specified by domain
+    Variant of @ref FrameSet::getFrame "getFrame(int, bool)" where the frame


This still links to the base class docs instead of the derived class docs, which is likely to confuse the reader. Same story below.

I changed the text in all cases. However, the links still go to the base class docs -- it appears that Doxygen does not list or provide docs for methods brought in via "using".

Strange, I'll investigate.

Thanks. If you can figure out why the link to getMapping(int, int) is failing or suggest an alternate syntax that works, that would be also be appreciated.

kfindeisen · 2017-11-08T01:39:49Z

include/astshim/FrameDict.h

-    or empty domain.
+    Build a new domain:index dict from a FrameSet
+    */
+    std::unordered_map<std::string, int> _makeNewDict(FrameSet const &frameSet) const;


Consider making this static.

kfindeisen · 2017-11-08T01:42:51Z

include/astshim/FrameDict.h

-        }
+    void _update(FrameSet &frameSet) {
+        _domainIndexDict = _makeNewDict(frameSet);
+        this->swapRawPointers(frameSet);


Might want to make it clearer (with documentation and/or parameter name) that frameSet is what will replace this object.

Might also be worth mentioning that this method is strongly exception-safe, since that guarantee is essential to _update's purpose. (You do say "if successful", but when reviewing FrameDict.cc I got the mistaken impression that _update modifies frameSet before doing the swap.)

kfindeisen · 2017-11-08T17:44:55Z

include/astshim/Object.h

+    Swap the raw object pointers between this and another object
+    */
+    void swapRawPointers(Object &other) {
+        std::swap(_objPtr, other._objPtr);


Bad style to explicitly use std::swap (see Effective C++, Item 25 for why this can exclude the best swap implementation), but unlikely to matter in practice because ObjectPtr is defined in C.

On the other hand, since construction and assignment of ObjectPtr are C operations, you can safely declare this method noexcept.

kfindeisen · 2017-11-08T18:02:10Z

include/astshim/detail/utils.h

 #include <stdexcept>
+#include <vector>


Why do you need to add <limits>, <sstream>, or <vector>?

They are used later in the file. That need is unrelated to this ticket; I only discovered it while working on this ticket. I'll make it a separate commit.

kfindeisen · 2017-11-08T18:11:29Z

src/FrameDict.cc

+    auto copy = getFrameSet();
+    copy->addFrame(iframe, map, frame);
+    _update(*copy);
+}


Very clean. 😄

However, this code relies on its correctness on getFrameSet() always returning an object whose type is exactly FrameSet (for example, if it returned a FrameDict you'd get infinite recursion). Given that methods in FrameSet have a history of later becoming virtual, consider documenting that getFrameSet() always returns a base FrameSet.

Good catch. I added proper documentation.

kfindeisen · 2017-11-08T19:01:56Z

tests/test_frameDict.py

@@ -28,12 +28,12 @@ def test_FrameDictOneFrameConstructor(self):
        frameDict = ast.FrameDict(self.frame1)
        self.assertIsInstance(frameDict, ast.FrameDict)
        self.assertEqual(frameDict.nFrame, 1)
-        self.assertEqual(frameDict.getAllDomains(), ["FRAME1"])
+        self.assertEqual(frameDict.getAllDomains(), set(["FRAME1"]))


Wouldn't {"FRAME1"} be more Pythonic?

OK. I changed all instances.

kfindeisen · 2017-11-08T19:05:38Z

tests/test_frameDict.py

        self.assertEqual(frameDict.getFrame("FRAME2").domain, "FRAME2")
        self.assertEqual(frameDict.getFrame(frameDict.CURRENT).domain, "FRAME2")
+        self.assertEqual(frameDict.getAllDomains(), set(["FRAME1", "FRAME2"]))


Again, why not say {"FRAME1", "FRAME2"}?

r-owen added 2 commits October 31, 2017 07:50

Make Object.test const

63474d3

Rename arguments for frameSet.getMapping

4f8fdb4

from "ind1, ind2" to "from, to"

r-owen force-pushed the tickets/DM-12452 branch from a4fceca to b09222d Compare October 31, 2017 15:24

kfindeisen requested changes Nov 2, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-12452 branch 3 times, most recently from 09661d5 to ce3f00f Compare November 8, 2017 00:56

kfindeisen approved these changes Nov 8, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-12452 branch from 4a032a8 to 58a9eaa Compare November 9, 2017 16:32

Add missing includes

e2d8aec

r-owen force-pushed the tickets/DM-12452 branch from 58a9eaa to d56ab0d Compare November 9, 2017 16:35

Add FrameDict class

6daaeba

r-owen force-pushed the tickets/DM-12452 branch from d56ab0d to 6daaeba Compare November 9, 2017 16:37

r-owen merged commit 6daaeba into master Nov 9, 2017

ktlim deleted the tickets/DM-12452 branch August 25, 2018 04:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-12452: Add FrameDict class #32

DM-12452: Add FrameDict class #32

r-owen commented Oct 31, 2017

kfindeisen left a comment

kfindeisen Nov 1, 2017

kfindeisen Nov 1, 2017

kfindeisen Nov 1, 2017

r-owen Nov 7, 2017

kfindeisen Nov 1, 2017

kfindeisen Nov 1, 2017

kfindeisen Nov 2, 2017 •

edited

r-owen Nov 7, 2017

kfindeisen Nov 2, 2017

kfindeisen Nov 2, 2017

r-owen Nov 7, 2017

kfindeisen Nov 2, 2017

kfindeisen Nov 2, 2017

kfindeisen Nov 2, 2017

kfindeisen left a comment

kfindeisen Nov 8, 2017

kfindeisen Nov 8, 2017

kfindeisen Nov 8, 2017

r-owen Nov 9, 2017 •

edited

kfindeisen Nov 9, 2017

r-owen Nov 9, 2017

kfindeisen Nov 8, 2017

kfindeisen Nov 8, 2017

kfindeisen Nov 8, 2017

kfindeisen Nov 8, 2017

r-owen Nov 9, 2017

kfindeisen Nov 8, 2017

r-owen Nov 9, 2017

kfindeisen Nov 8, 2017

r-owen Nov 9, 2017

kfindeisen Nov 8, 2017

		self.assertEqual(frameDict.getIndex("NEWFRAME1"), 1)
		self.assertEqual(frameDict.getIndex("NEWFRAME2"), 2)

DM-12452: Add FrameDict class #32

DM-12452: Add FrameDict class #32

Conversation

r-owen commented Oct 31, 2017

kfindeisen 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

kfindeisen Nov 2, 2017 • edited

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

kfindeisen 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

r-owen Nov 9, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen Nov 2, 2017 •

edited

r-owen Nov 9, 2017 •

edited