DM-8440: Create new Wcs class #9

r-owen · 2017-04-27T17:13:37Z

No description provided.

PaulPrice

Some minor comments.

This is my first time looking through astshim, and I found the few breakages of our naming rules (e.g., FitsChan and getNcard) rather annoying. I thought the whole idea of a shim would mean that our naming rules would apply.

PaulPrice · 2017-04-27T19:19:24Z

python/astshim/fitsChan.cc

@@ -24,6 +24,7 @@

 #include <pybind11/pybind11.h>
 #include <pybind11/complex.h>
+#include <pybind11/stl.h>


Looks unrelated.

It is needed because getCardNames returns a vector. pybind11/stl.h wraps C++ vectors so they are usable in Python.

PaulPrice · 2017-04-27T19:23:46Z

include/astshim/FitsChan.h

+    Not "const" because the code changes the index of the current card while operating
+    (but restores the original index when done).
+    */
+    std::vector<std::string> getCardNames();


This method differs only by a single character from the one before. Might people get confused? The previous one might be better named getCurrentCardName.
Could you make the card index volatile and make this method const?

The names are based on AST. astshim is a 3rd party package intended to be a thin shim around AST. As such I felt minimum friction was to follow AST where possible, so the extensive AST documentation would still apply.

But astshim isn't a third-party package: it's hosted here in LSST's GitHub, and I'm reviewing code changes for it. But maybe I'm just unaware of the context.

I'm hoping we'll integrate it with AST upstream to make it part of the normal C++ interface for AST.

I was treating astshim as a temporary package name rather than a permanent package name for LSST.

If astshim is going into AST, then shouldn't we have our own wrappers that insulate us from changes in that? All of this seems to be in this strange no man's land, not quite LSST, but not quite AST. We're exposing far more of AST than we do with any other package, including Eigen.

We can easily switch {{astshim}} names to use LSST capitalization, e.g. {{getNAxes}} instead of {{getNaxes}}. That will remove some of the pain but certainly not all.

Getting back to your first comment...I agree that {{getCardNames}} could be confused with {{getCardName}} though I was hoping we could live with it. Is it worth changing to {{getAllCardNames}} or do you have a better suggestion?

{{FitsChan}} makes heavy use of its index and the concept of the current card, so I think it will take more work than it's worth to make this method const.

And...I should have said: {{getCardName}} is from AST so I don't want to change it. A method to return all card names in order does not exist in AST so I can easily rename that.

If this package is indeed bound for integration into AST and afw isn't exposing any of this, then I won't object to your naming choices. However, I do think it's worth considering getAllCardNames or something else that will distinguish it from getCardName.

PaulPrice · 2017-04-27T19:45:16Z

tests/test_fitsChan.py

+        self.assertEqual(fc.getNcard(), 1)
+        self.assertEqual(fc.getCardNames(), ["FRED1"])
+        fc.setFitsCF("FRED1", complex(99.9, 99.8), "Hello there", True)
+        fv = fc.getFitsCF("FRED1")


Why did this change?

I felt the test didn't need all those extra cards. The first new card was informative (e.g. FRED1 and FRED2), but the rest (FRED3 and FRED4) did not enhance the test, and reusing a name is also a useful test.

PaulPrice · 2017-04-27T19:45:31Z

tests/test_fitsChan.py

-        fc.setFitsS("FRED4", "-12", "Hello there", True)
-        fv = fc.getFitsI("FRED4")
+        fc.setFitsS("FRED1", "-12", "Hello there", True)
+        fv = fc.getFitsI("FRED1")


PaulPrice · 2017-04-27T19:54:24Z

include/astshim/Frame.h

-    (as a Mapping) to transform coordinate values in either direction.
+    If conversion is possible, it returns a shared pointer to a @ref FrameSet which describes
+    the conversion and which may be used (as a Mapping) to transform coordinate values in either direction.
+    Otherwise it returns an empty shared pointer.


I think there are cases where you want the exception and not an empty pointer. Ideally, one wants both modes. Can you put in an overload that will throw?

I agree. I'll add a flag or a new method. It should be a custom exception so it's easy to differentiate "not found" from other errors.

On second thought I want to hold off on this. I think users will often want to control the exception being raised and the message string when a search fails. Furthermore it is not idiomatic C++ to raise an exception when a search fails.

PaulPrice · 2017-04-27T19:58:11Z

include/astshim/Frame.h

@@ -769,7 +767,7 @@ class Frame : public Mapping {
    using a "domainlist" string which does not include the template's domain
    (or a blank field). If you do so, no coordinate system will be found.
    */
-    FrameSet findFrame(Frame const &tmplt, std::string const &domainlist = "");
+    std::shared_ptr<FrameSet> findFrame(Frame const &tmplt, std::string const &domainlist = "");


Why spaces around the = here but not above?

clang-format. Apparently I didn't run it on the whole file.

I will run clang-format on all C++ code.

PaulPrice · 2017-04-27T19:59:48Z

src/Frame.cc

    }
-    return FrameSet(rawFrameSet);
+    return Object::fromAstObject<FrameSet>(reinterpret_cast<AstObject *>(rawFrameSet), false);    


Trailing whitespace.
And line 47 too.

I'll run clang-format on both Frame.cc and Frame.h

and test the new method. I took the opportunity to slightly simplify one of the test cases: a card was being replaced without testing that it was truly being replaced, and the card name changed more often than necessary to show that replacement works.

Change `Frame.convert` and `Frame.findFrame` to return `std::shared_ptr<FrameSet>` instead of `FrameSet` and return an empty pointer if the search is unsuccessful instead of throwing notfound_error. This is more useful for the user and closer to how AST does it. It also eliminates the need for notfound_error and avoids raising a C++ exception for a normal situation.

Document the effect that `FrameSet.removeFrame` and `FrameSet.addFrame` have on the indices of the various frames.

PaulPrice reviewed Apr 27, 2017

View reviewed changes

r-owen force-pushed the tickets/DM-8440 branch from 924e1e3 to 578a735 Compare May 2, 2017 18:33

r-owen added 6 commits May 2, 2017 11:45

Add FitsChan::getAllCardNames

e38c9b7

and test the new method. I took the opportunity to slightly simplify one of the test cases: a card was being replaced without testing that it was truly being replaced, and the card name changed more often than necessary to show that replacement works.

Add copy argument to FrameSet.getFrame

fab7fb1

Improve FrameSet.removeFrame & addFrame docs

dca6807

Document the effect that `FrameSet.removeFrame` and `FrameSet.addFrame` have on the indices of the various frames.

Add missing constant to FrameSet.cc

3fc5737

Run clang-format on all C++ code

b752128

r-owen force-pushed the tickets/DM-8440 branch from 578a735 to 3fc5737 Compare May 2, 2017 19:02

r-owen merged commit b752128 into master May 2, 2017

ktlim deleted the tickets/DM-8440 branch August 25, 2018 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-8440: Create new Wcs class #9

DM-8440: Create new Wcs class #9

r-owen commented Apr 27, 2017

PaulPrice left a comment

PaulPrice Apr 27, 2017

r-owen Apr 27, 2017

PaulPrice Apr 27, 2017

r-owen Apr 27, 2017

PaulPrice Apr 27, 2017

timj Apr 27, 2017

timj Apr 27, 2017

PaulPrice Apr 28, 2017

r-owen May 2, 2017

r-owen May 2, 2017

r-owen May 2, 2017

PaulPrice May 2, 2017

PaulPrice Apr 27, 2017

r-owen Apr 27, 2017

PaulPrice Apr 27, 2017

PaulPrice Apr 27, 2017

r-owen Apr 27, 2017

r-owen May 2, 2017

PaulPrice Apr 27, 2017

r-owen Apr 27, 2017

r-owen May 2, 2017

PaulPrice Apr 27, 2017

r-owen May 2, 2017

DM-8440: Create new Wcs class #9

DM-8440: Create new Wcs class #9

Conversation

r-owen commented Apr 27, 2017

PaulPrice 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

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