DM-41634: Add PixelMap subclass using AST mappings #15

cmsaunders · 2023-11-27T20:29:05Z

No description provided.

mwittgen · 2023-11-28T02:44:46Z

astrometry/include/PixelMap.h

@@ -17,6 +17,8 @@
 #ifdef USE_YAML
 #include "yaml-cpp/yaml.h"
 #endif
+#include "astshim.h"
+
 #include "Std.h"


This is ugly in the original gbdes code that Std.h
Include all std headers and adding a using std

Yeah, it's not great, but we decided not to go through and change all the original code. New code is supposed to ignore this, but I forgot in the instance you commented on below.

mwittgen · 2023-11-28T02:46:29Z

astrometry/include/PixelMap.h

+  class ASTMap: public PixelMap {
+    // Class representing a constant shift of position
+  public:
+    ASTMap(const ast::Mapping& mapping_, string name_=""):


This is std::string obfuscated by the using std

Thanks for catching this. We've been trying to include std:: in all new code, but I forgot and copied what was elsewhere in this file.

mwittgen · 2023-11-28T03:13:49Z

astrometry/include/PixelMap.h

+
+    virtual PixelMap* duplicate() const;
+
+    ~ASTMap() {}


can be declared as default dtor

Suggested change

~ASTMap() {}

~ASTMap() = default;

mwittgen · 2023-11-28T03:25:53Z

CMakeLists.txt

+astshim.h REQUIRED
+HINTS $ENV{ASTSHIM_DIR}/include
+)
+


I can't comment on this line below
This adds the target WCSfit as an executable
linking all static libs. astrometry which links astshim
Linking everything into one shlib gbdes might work and link this to this executable. Or remove
this target if it is not needed.

Suggested change

# add_subdirectory(src)

I don't think we need an executable target here, unless @cmsaunders still finds it convenient for comparison-testing sometimes.

I'm going to just leave in instructions for how to build the executable if needed.

TallJimbo

One possibly-major issue, but it should be straighftforward to fix.

TallJimbo · 2023-11-30T15:42:26Z

CMakeLists.txt

+astshim.h REQUIRED
+HINTS $ENV{ASTSHIM_DIR}/include
+)
+


I don't think we need an executable target here, unless @cmsaunders still finds it convenient for comparison-testing sometimes.

TallJimbo · 2023-11-30T15:43:37Z

astrometry/include/PixelMap.h

+               double &xpix, double &ypix,
+               double color = astrometry::NODATA) const;
+
+#ifdef USE_YAML


Do you know if USE_YAML is actually set in our builds? If not, it might be cleaner to leaf this block out entirely.

Yes, yaml is built into the mappings and if USE_YAML is false some key classes don't get built. Long term, it would be nice to remove this dependence though.

TallJimbo · 2023-11-30T15:44:10Z

astrometry/include/PixelMap.h

+
+    static string type() {return "AST";}
+
+    ast::Mapping mapping;


It would be better for this to be private.

TallJimbo · 2023-11-30T15:51:54Z

astrometry/include/PixelMap.h

+    // Class representing a constant shift of position
+  public:
+    ASTMap(const ast::Mapping& mapping_, std::string name_=""):
+      PixelMap(name_), mapping(mapping_) {}


I think it would be much better to make the mapping data member a std::shared_ptr<ast::Mapping> and call copy() here. You don't actually care about being able to share pointers, and all else being equal I'd prefer unique_ptr, but once you have a shared_ptr (and that's what methods like Mapping::copy return) you're stuck with it. The important thing is that the mapping_ could be an instance of a derived class of Mapping, and by copy-constructing it you're slicing off any extra stuff the derived class might have added.

Normally that would result in really horrible bugs, and the copy constructor would have been disabled to protect against that, and I find it strange that that's not the case in Mapping. I'm sure it's related to the fact that the real inheritance is happening in AST C code and the C++ inheritance is just a facade around that, but I still think it's much safer to hold by pointer and avoid the type-slicing yourself.

I think I've implemented what you suggest. Can you check and see if I did what you meant? I also changed duplicate so that it no longer does a copy.

TallJimbo · 2023-11-30T15:53:34Z

pydir/wcsfit.cpp

@@ -102,6 +103,10 @@ PYBIND11_MODULE(wcsfit, m) {
    py::class_<astrometry::SubMap, astrometry::PixelMap>(m, "SubMap")
            .def(py::init<list<astrometry::PixelMap *> const &, std::string, bool>());

+    py::class_<astrometry::ASTMap, astrometry::PixelMap>(m, "ASTMap")
+            .def(py::init<ast::Mapping, std::string>(),


Suggested change

.def(py::init<ast::Mapping, std::string>(),

.def(py::init<ast::Mapping const &, std::string>(),

If this isn't marked as a reference or pointer we'll get more type-slicing in the pybind11 layer.

timj

Looks okay in general. I'm worried about performance issues since this is a very non-optimal way of using AST with one transformation at a time.

timj · 2023-11-30T18:13:36Z

astrometry/include/PixelMap.h

@@ -332,5 +334,37 @@ namespace astrometry {
    void operator=(const ColorTerm& rhs) = delete;

  };
+
+  class ASTMap: public PixelMap {
+    // Class representing a constant shift of position


I don't understand why the ASTMap class is described as representing solely a constant shift.

Just a mistake due to my using a different class as a template.

timj · 2023-11-30T18:22:46Z

astrometry/include/PixelMap.h

+
+#ifdef USE_YAML
+    // AST maps are not expected to be used for output, so we do not
+    // attempt to write a full description to yaml.


We could, because AST mappings can be serialized to ASDF YAML.

Good to know. I'm not going to implement that now, because I think it's pretty unlikely it will be needed.

timj · 2023-11-30T18:27:07Z

astrometry/src/PixelMap.cpp

+     double color) const {
+
+  std::vector<double> pixCoords{xpix, ypix};
+  std::vector<double> worldCoord = mapping.applyForward(pixCoords);


I'm a bit worried that this is going to be slow. AST is much faster if you give it many coordinates to transform and it's even faster if you ask it to transform a regular pixel grid to WCS in one go (astMapGrid).

I did a timing test, and there is a slow-down, but it's not prohibitive. This will only be used in a couple places in setting up the astrometric fit. For 31 visits, the time it took to do source association went from ~4 seconds to ~7 seconds, and the fit initialization went from ~33 seconds to ~41 seconds.

Ok. I assume it's not possible to run this in bulk given the way gbdes calls the transformation code, but I imagine it would pull all that time back if you did that.

TallJimbo

One small change requested with the new version, but the rest looks good.

TallJimbo · 2023-12-01T18:26:08Z

astrometry/src/PixelMap.cpp

+
+PixelMap*
+ASTMap::duplicate() const {
+  return new ASTMap(*mapping);


Suggested change

return new ASTMap(*mapping);

return new ASTMap(mapping->copy());

We want this to be a real deep copy, not a shallow copy of just the shared_ptr.

Two things--first isn’t mapping getting copied twice this way, because it gets copied again in the constructor? Second, should the constructor also be changed for this? As suggested, it doesn't compile
(no known conversion for argument 1 from 'std::shared_ptr<ast::Mapping>' to 'const ast::Mapping&').

You're right - the copy is already there in the constructor, and we don't need another copy here. Ignore me.

Add PixelMap subclass using AST mappings

08a7f4f

cmsaunders force-pushed the tickets/DM-41634 branch from b9b48c3 to 08a7f4f Compare November 27, 2023 20:41

cmsaunders added 2 commits November 27, 2023 14:18

Add astshim in pydir CMakeLists include dirs

e5f2b52

Add include astshim back in

064e686

mwittgen reviewed Nov 28, 2023

View reviewed changes

cmsaunders added 2 commits November 28, 2023 08:44

Remove src/ build and small PixelMap.h edits

4b51838

Add comment for building executable

8278526

cmsaunders force-pushed the tickets/DM-41634 branch from b88d245 to 8278526 Compare November 29, 2023 17:20

TallJimbo approved these changes Nov 30, 2023

View reviewed changes

timj approved these changes Nov 30, 2023

View reviewed changes

Make mapping private and a shared_ptr

c7ff475

TallJimbo approved these changes Dec 1, 2023

View reviewed changes

cmsaunders merged commit b5d82f1 into lsst-dev Dec 4, 2023

cmsaunders deleted the tickets/DM-41634 branch December 4, 2023 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-41634: Add PixelMap subclass using AST mappings #15

DM-41634: Add PixelMap subclass using AST mappings #15

cmsaunders commented Nov 27, 2023

mwittgen Nov 28, 2023 •

edited

cmsaunders Nov 28, 2023

mwittgen Nov 28, 2023

cmsaunders Nov 28, 2023

mwittgen Nov 28, 2023

mwittgen Nov 28, 2023

TallJimbo Nov 30, 2023

cmsaunders Dec 1, 2023

TallJimbo left a comment

TallJimbo Nov 30, 2023

TallJimbo Nov 30, 2023

cmsaunders Dec 1, 2023

TallJimbo Nov 30, 2023

TallJimbo Nov 30, 2023

cmsaunders Dec 1, 2023

TallJimbo Nov 30, 2023

timj left a comment

timj Nov 30, 2023

cmsaunders Dec 1, 2023

timj Nov 30, 2023

cmsaunders Dec 1, 2023

timj Nov 30, 2023

cmsaunders Dec 1, 2023

timj Dec 1, 2023

TallJimbo left a comment

TallJimbo Dec 1, 2023

cmsaunders Dec 1, 2023

TallJimbo Dec 1, 2023

	.def(py::init<ast::Mapping, std::string>(),
	.def(py::init<ast::Mapping const &, std::string>(),

	return new ASTMap(*mapping);
	return new ASTMap(mapping->copy());

DM-41634: Add PixelMap subclass using AST mappings #15

DM-41634: Add PixelMap subclass using AST mappings #15

Conversation

cmsaunders commented Nov 27, 2023

mwittgen Nov 28, 2023 • 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

TallJimbo 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

timj 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

TallJimbo 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

mwittgen Nov 28, 2023 •

edited