DM-31226: Add functionality needed for WCSFitTask in drp_tasks #9

cmsaunders · 2022-10-06T00:09:24Z

This PR adds the following functionality:

Conversion from SIP to TPV for the input WCS
More information about the pixel maps is exposed to the Python wrapper
Options to output catalogs with residuals, fit star positions, etc., in the form of dictionaries that can be trivially converted to Pandas Dataframes.
Improvements in interface for fitting proper motions and parallax.

natelust

There is some work to be done here, and I would be happy to weight in on any responses, or look again if you wish, but I am also happy for you to take these as advisement and fix it up how you decide.

natelust · 2022-11-09T16:18:47Z

astrometry/include/PixelMap.h

@@ -256,6 +257,56 @@ namespace astrometry {
    virtual void write(YAML::Emitter& os) const;
 #endif
    virtual string getType() const {return type();}
+    Matrix33 getPixMatrix() {
+      cerr << "start get pix" << endl;


Do we normally send things out to std err vs logging? Or is this just because our gbdes forking?

There's a separate ticket to move to proper logging. However, in this case this should just be removed.

natelust · 2022-11-09T16:28:43Z

astrometry/include/PixelMap.h

@@ -256,6 +257,56 @@ namespace astrometry {
    virtual void write(YAML::Emitter& os) const;
 #endif
    virtual string getType() const {return type();}
+    Matrix33 getPixMatrix() {


Why are you defining all of this in the header, instead of the signature here and the body in a src file?

I probably did this before I read that section in the dev guide...will fix.

natelust · 2022-11-09T16:30:36Z

astrometry/include/PixelMap.h

+      auto gnom = dynamic_cast<const astrometry::Gnomonic*>(pix);
+      cerr << "did dyn cast" << endl;
+      Matrix33 tmpMatrix;
+      tmpMatrix.setToIdentity();


would it make sense to do this with tmpMatrix only if it is actually going to be needed? (same below)

natelust · 2022-11-09T16:34:54Z

astrometry/include/PixelMap.h

+        if (!icrs) {
+          Vector2 lonlat;
+          cerr << "not gnom or icrs" << endl;
+          return lonlat;


I'm not familiar with this object, is lonlat a class level variable?

I just removed this function, because it was something I used for debugging, and I don't think I will need it again.

natelust · 2022-11-09T16:40:09Z

astrometry/include/PixelMap.h

+        return icrs->getLonLat();
+      }
+      SphericalICRS pole = gnom->getOrient()->getPole();
+      Vector2 lonlat = pole.getLonLat();


if lonlat is a class level variable name, maybe it would be a good idea to use a different variable name here for human readability

natelust · 2022-11-15T18:08:20Z

src/subs/FitSubroutines.cpp

-            vv[astrometry::VX] *= units[i];
-            vv[astrometry::VY] *= units[i];
-            vv[astrometry::PAR] *= units[i];
+            vv[astrometry::VX] *= units[astrometry::VX];


I cant tell if this is a github rendering, but is this inside the for loop?

Yes. I changed this because I saw that i was being reused as an index. I guess it wasn't affecting the i in the outer scope, because I think the vector was still being filled, but it seems like bad practice.

natelust · 2022-11-15T18:11:40Z

src/subs/FitSubroutines.cpp

+                projection = catalogProjections[detptr->catalogNumber];
+                if (projection) break;
+            }
+            if (projection) {


couldn't you save this branch if you just did this work inside line 2772, and did break at the end?

natelust · 2022-11-15T18:13:41Z

src/subs/TPVMap.cpp

+    // Fill ac and bc
+    for (int m = 0; m <= a_order; m++) {
+        for (int n = 0; n <= (a_order - m); n++) {
+            std::string keyString = "A_" + std::to_string(m) + "_" + std::to_string(n);


is addition the best way to do this? or is there a standard formatting?

This seems like the simplest way in c++17. I spent some time looking and didn't see better options, but please tell me if I'm missing something.

natelust · 2022-11-15T18:28:00Z

src/subs/TPVMap.cpp

+    // Create a SubMap that owns a duplicate of these one or two maps (no sharing):
+    SubMap sm(pmlist, name, false);
+    // Delete the polymap if we made it:
+    delete pv;


I'm not sure I follow all the need for allocating a new object, and freeing like this. Couldn't this be refactored to not need the dynamic memory like this? perhaps by directly creating the SubMap?

I basically copied what is in readTPV. We can discuss how to refactor.

natelust · 2022-11-15T18:29:47Z

src/subs/WCSFitRoutine.cpp

@@ -352,11 +365,15 @@ void WCSFit::setObjects(int i, const map<std::string, std::vector<double>> &tabl
                        const std::string &idKey, const std::string &pmCovKey, const std::string &magKey,
                        const int &magKeyElement, const std::string &magErrKey, const int &magErrKeyElement,
                        const std::string &pmRaKey, const std::string &pmDecKey,
-                        const std::string &parallaxKey) {
+                        const std::string &parallaxKey, const std::vector<std::vector<double>> &pmCov) {


Does this parameter have a default in a header somewhere?

Yeah, it defaults to zero. Why do you ask, because it changes the API?

cmsaunders added 5 commits October 5, 2022 09:05

Allow options to start with empty inputYAML and Composite exposure model

177760a

Expose more information about pixel maps

2c2e6c9

Add function to make TPV WCS from SIP input

bf03683

Add PM to wrapper and add fns to return catalogs

454c48c

Remove old test files

5cc2a90

natelust approved these changes Nov 15, 2022

View reviewed changes

cmsaunders added 2 commits November 18, 2022 08:01

Respond to referee comments

0f525c1

Switch gbfits to static library

b11d828

cmsaunders merged commit ce05b32 into lsst-dev Dec 16, 2022

cmsaunders deleted the tickets/DM-31226 branch December 16, 2022 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-31226: Add functionality needed for WCSFitTask in drp_tasks #9

DM-31226: Add functionality needed for WCSFitTask in drp_tasks #9

cmsaunders commented Oct 6, 2022 •

edited

natelust left a comment

natelust Nov 9, 2022

cmsaunders Nov 17, 2022

natelust Nov 9, 2022

cmsaunders Nov 17, 2022

natelust Nov 9, 2022

natelust Nov 9, 2022

cmsaunders Nov 17, 2022

natelust Nov 9, 2022

cmsaunders Nov 17, 2022

natelust Nov 15, 2022

cmsaunders Nov 17, 2022

natelust Nov 15, 2022

natelust Nov 15, 2022

cmsaunders Nov 17, 2022

natelust Nov 15, 2022

cmsaunders Nov 17, 2022

natelust Nov 15, 2022

cmsaunders Nov 17, 2022

DM-31226: Add functionality needed for WCSFitTask in drp_tasks #9

DM-31226: Add functionality needed for WCSFitTask in drp_tasks #9

Conversation

cmsaunders commented Oct 6, 2022 • edited

natelust 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

cmsaunders commented Oct 6, 2022 •

edited