wiki:Winter2014/Bosch/Miscellaneous
Last modified 5 years ago Last modified on 10/21/2013 10:50:03 AM

Bosch's Refactoring Thoughts: Miscellaneous

The complaints and proposals here don't necessarily represent small changes (though some do); they represent things I don't have a lot to say about, because I haven't had time to dream up a complete proposal or look too deeply into the requirements.

General

  • We should have a consistent API for dynamic casts in Python, and a Swig macro to enable it. Currently, some classes have a .cast() static method, while others have a .swigConvert(), and there may be others I'm not aware of.
  • We should enable keyword arguments in Swig. I think this is a build option we can just turn on, but it may lead to other complications we'd then need to fix. Unfortunately, this would only work for non-overloaded methods, but overloads are where we need keywords the most. For overloaded methods and constructors that are called extremely frequently and have many overloads (i.e. the Image ctors), we may want to add a manual %shadow block to add keyword argument support manually. We could also move away from overloads in some cases, for instance by replacing public overloaded constructors with static method factories ("named constructors").
  • We should have a consistent clone pattern, and decide whether it should be present for nonpolymorphic objects as well as polymorphic objects. Ideally, I think we'd do this after C++11, and have all clone methods return unique_ptr<Derived> aside from hierarchies that need shared_from_this support. To be able to return pointers to Derived rather than pointers to Base, we'd need to have nonvirtual public methods as well as virtual protected methods, but we could probably define a macro to reduce the boilerplate. Here's how it would work without the macros:
    class Base {
    public:
        unique_ptr<Base> clone() const { return _clone(); }
    protected:
        virtual unique_ptr<Base> _clone() const = 0;
    };
    
    class Derived : public Base {
    public:
        unique_ptr<Derived> clone() const { return static_pointer_cast<Derived>(_clone()); }
    private:
        virtual unique_ptr<Base> _clone() const;
    };
    
  • We should use namespace blocks in .cc files instead of aliases in .cc files, and remove the ambiguity in our code standards here.

Documentation

  • We should move Doxygen blocks from older .cc files to .h files, but re-enable running doxygen on the source files.
  • We should remove @file Doxygen blocks, or at least the @author tags in these - most of these now refer to authors who have left the project, and "git blame" is a much better way of determine authorship anyway.
  • We should have a policy for how to use various Doxygen grouping mechanisms, with a focus on making sure the cross-package Doxygen makes sense with them.

Package Reorganization

While we developers are all familiar with the names of packages right now, new developers coming onboard during construction and future users will be confused by the seemingly arbitrary prefixes for many of our packages, and the grab-bag nature of several afw subpackages. A major part of our code audit and review plans should involve renaming and reorganizing some packages:

  • We should eliminate the daf_, pex_, ctrl_, ip_, and afw prefixes; I don't believe these convey any useful information. We should repurpose the meas prefix so that it actually does mean "measurement" rather than "apps stuff that sits above afw and isn't ip_*".
  • We should consider merging base, daf_base and utils. We could also consider moving exceptions and possibly logging into this package; I don't think there's a lot of value in having them separate, if all higher-level packages depend on all of these.
  • We should rename the standalone "geom" package to something that implies its purpose as a spherical geometry package, and let "geom" be what is currently afw::geom.
  • We should move a Wcs and Coord into the same package; perhaps the spherical geometry package hopefully-soon-to-be-formally-known-as "geom".
  • We should audit and reorganize the content in the math and detection subpackages of afw and meas_algorithms, and come up with a grouping that moves related things closer (e.g. Kernel and Psf) while minimizing circular dependencies.

afw::image

Coordinate Conventions

Our image classes currently handle two different pixel coordinate systems which differ only by the offset commonly referred to as "xy0". The PARENT coordinate system puts the first pixel in the image at xy0; the LOCAL coordinate system always puts the first pixel at (0,0).

Our general rule has long been "always pay attention to xy0", which implies PARENT, but the Image class itself doesn't: in many operations, including subimage accessors and bbox getters, LOCAL is the default, whereas in others - particularly the pixel and iterator accessors - there isn't even an option to use PARENT. IMO this is the main source of image-offset bugs in the code.

We should at least make PARENT the default for all Image member functions, and consider removing LOCAL entirely. This is a simple change to make in the Image classes themselves, but it will break a lot of code in possibly subtle ways. We should consider renaming all offending methods when making this change, forcing downstream code to break, and then renaming them back when everything downstream has been fixed.

Other

  • Consider dropping boost::gil in favor of a couple of custom iterators; I'm not sure we're getting much from gil now that we're using ndarray as a backend.
  • I don't believe the Image classes need to have virtual dtors.
  • We return pointers to images in many places where this is unnecessary; returning a copy of an Image is always cheap (even when copy elision doesn't work) because the copies are shallow.

Footprints

I have a big overhaul of Footprints that was proposed and design-reviewed back in Winter2012, which can be found here. I haven't looked at the page in a long time, so it may be a bit out of date. But not too much has changed on the Footprints stuff, and I still think these changes are worth doing (they didn't get done before just because of time constraints). In particular:

  • Footprint should be based on a container of Spans, not a container of shared_ptr to Span; the size of a Span is smaller than the size of a shared_ptr on most platforms, and we have a lot of them.
  • We need something like FootprintFunctor but without the need for a virtual function call on every pixel. I think this old proposal addresses this as well.

Kernel and Function classes

  • Could we remove NullFunction in favor of null pointers / None?
  • It looks like IntegerDeltaFunction1, IntegerDeltaFunction2 are unused.
  • Should use the Angle class where possible (in some cases, it'd be even better to switch to using afw::geom::ellipses in e.g. 2-d Gaussian functions).
  • Warping kernels seem very different from the other kernels; perhaps they should be a different hierarchy (though we'd still want to share SeparableKernel? code somehow). For instance, non-warping Kernels should always have odd sizes and don't (I think) need setCtr.

Backgrounds

  • The background model should be something we can attach to and store with an Exposure.

afw::geom

  • Better stringification.
  • Use Angle in afw::geom::ellipses, improve documentation, fix singularities where possible (#2600, #2828).
  • Add polygon class.
  • Add (possibly in a new namespace) C++ implementations of spherical geometry stuff from "geom" package. Probably want different classes for Cartesian geometry objects and spherical geometry objects, but consistency in the APIs where appropriate.

skymap

  • Unique numeric IDs for each patch (probably populated by autoincrement upon construction, instead of bit-allocated).
  • String names that can serve as aliases for tracts: if we have a DiscreteSkyMap that represents several deep fields, it'd be nice to be able to name them.
  • It'd be nice to be able to have multiple kinds of tracts within a single SkyMap (i.e. we could put a full-sky survey and a few deep fields in the same skymap, with the deep fields just being some extra "special" tracts).
  • We'd do this by making TractInfo class the polymorphic, inheritable instead of the SkyMap class, which would then just be a fancy container for heterogeneous TractInfo objects.
  • This would undoubtedly complicate persistence etc., but I think it'd be worth doing.

pex_exceptions

  • Remove "Exception" suffix from exception names.
  • Allow LSST_THROW to take a boost::format message directly.
  • Find a way to remove LsstCppException and make Python exceptions better mirror the C++ ones. We may be able to use Swig's %shadow directive to make the Python classes inherit from Python's built-in base Exception class, but if not we can create the special shadow classes ourselves via a custom Swig macro, instead of letting Swig wrap them as usual. It'd be some tricky work up front, and it may require a little more boilerplate for each exception (i.e. a Swig macro invocation in the .i file), but it'd be worthwhile overall.
  • Add assert functions that raise instead of throw to replace "if (...) throw" constructs (i.e. #2965).
  • Audit our current exception class hierarchy, and add clear guidelines for when different exceptions should be used.