Skip to content

Commit

Permalink
Replace FITS I/O functions with overloads that don't use shared_ptr.
Browse files Browse the repository at this point in the history 
The original versions of these functions violate rule 5-24b of the
LSST C++ style guide and/or rule F7 of the C++ Core Guidelines: they
take a smart pointer argument but don't actually need to, because they
don't take ownership of the object or otherwise manipulate the
object's lifetime.  This is a viral problem, because it tempts
downstream code to use smart pointers unnecessarily in arguments that
will be forwarded to these.

There are many more examples of this, but they'll be easier to resolve
with or after RFC-840.
  • Loading branch information
@TallJimbo
TallJimbo committed Mar 25, 2022
1 parent a770eb1 commit 9715b4d
Show file tree
Hide file tree
Showing 11 changed files with 335 additions and 109 deletions.
23 changes: 18 additions & 5 deletions include/lsst/afw/fits.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,7 @@ class Fits {
writeImageImpl(makeContiguousArray(array).getData(), array.getNumElements());
}

//@{
/**
* Write an image to FITS
*
Expand All @@ -493,9 +494,15 @@ class Fits {
template <typename T>
void writeImage(
image::ImageBase<T> const& image, ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> header = std::shared_ptr<daf::base::PropertyList>(),
std::shared_ptr<image::Mask<image::MaskPixel> const> mask =
std::shared_ptr<image::Mask<image::MaskPixel>>());
daf::base::PropertySet const * header = nullptr,
image::Mask<image::MaskPixel> const * mask = nullptr);
template <typename T>
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeImage(
image::ImageBase<T> const& image, ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> header = nullptr,
std::shared_ptr<image::Mask<image::MaskPixel> const> mask = nullptr);
//@}

/// Return the number of dimensions in the current HDU.
int getImageDim();
Expand Down Expand Up @@ -651,6 +658,7 @@ class Fits {
int behavior; // bitwise OR of BehaviorFlags
};

//@{
/**
* Combine two sets of metadata in a FITS-appropriate fashion
*
Expand All @@ -669,8 +677,13 @@ class Fits {
* - names in `second`, omitting "COMMENT" and "HISTORY" if valid versions appear in `first`
*/
std::shared_ptr<daf::base::PropertyList> combineMetadata(
std::shared_ptr<const daf::base::PropertyList> first,
std::shared_ptr<const daf::base::PropertyList> second);
daf::base::PropertyList const & first,
daf::base::PropertyList const & second);
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
std::shared_ptr<daf::base::PropertyList> combineMetadata(
std::shared_ptr<daf::base::PropertyList const> first,
std::shared_ptr<daf::base::PropertyList const> second);
//@}

/** Read FITS header
*
Expand Down
11 changes: 9 additions & 2 deletions include/lsst/afw/fitsCompression.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,11 +410,18 @@ class ImageScalingOptions {
/// @param[in] mask Mask for image (used to measuring statistics)
template <typename T>
ImageScale determine(image::ImageBase<T> const& image,
std::shared_ptr<image::Mask<image::MaskPixel> const> mask = nullptr) const {
image::Mask<image::MaskPixel> const * mask = nullptr) const {
auto const arrays = _toArray(image, mask);
return determine(arrays.first, arrays.second);
}

template <typename T>
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
ImageScale determine(image::ImageBase<T> const& image,
std::shared_ptr<image::Mask<image::MaskPixel> const> mask) const {
return determine(image, mask.get());
}

template <typename T, int N>
ImageScale determine(ndarray::Array<T const, N, N> const& image,
ndarray::Array<bool, N, N> const& mask) const;
Expand All @@ -425,7 +432,7 @@ class ImageScalingOptions {
template <typename T>
std::pair<ndarray::Array<T const, 2, 2>, ndarray::Array<bool, 2, 2>> _toArray(
image::ImageBase<T> const& image,
std::shared_ptr<image::Mask<image::MaskPixel> const> mask = nullptr) const {
image::Mask<image::MaskPixel> const * mask = nullptr) const {
if (mask && image.getDimensions() != mask->getDimensions()) {
std::ostringstream os;
os << "Size mismatch between image and mask: ";
Expand Down
69 changes: 56 additions & 13 deletions include/lsst/afw/image/Image.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ class Image : public ImageBase<PixelT> {

using ImageBase<PixelT>::operator[];

//@{
/**
* Write an image to a regular FITS file.
*
Expand All @@ -233,10 +234,15 @@ class Image : public ImageBase<PixelT> {
* @param[in] mode "w"=Create a new file; "a"=Append a new HDU.
*/
void writeFits(std::string const& fileName,
std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet const>(),
daf::base::PropertySet const * metadata = nullptr,
std::string const& mode = "w") const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(std::string const& fileName,
std::shared_ptr<daf::base::PropertySet const> metadata,
std::string const& mode = "w") const;
//@}

//@{
/**
* Write an image to a FITS RAM file.
*
Expand All @@ -245,19 +251,27 @@ class Image : public ImageBase<PixelT> {
* @param[in] mode "w"=Create a new file; "a"=Append a new HDU.
*/
void writeFits(fits::MemFileManager& manager,
std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet const>(),
daf::base::PropertySet const * metadata = nullptr,
std::string const& mode = "w") const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::MemFileManager& manager,
std::shared_ptr<daf::base::PropertySet const> metadata,
std::string const& mode = "w") const;
//@}

//@{
/**
* Write an image to an open FITS file object.
*
* @param[in] fitsfile A FITS file already open to the desired HDU.
* @param[in] metadata Additional values to write to the header (may be null).
*/
void writeFits(fits::Fits& fitsfile, std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet const>()) const;
void writeFits(fits::Fits& fitsfile, daf::base::PropertySet const * metadata = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::Fits& fitsfile, std::shared_ptr<daf::base::PropertySet const> metadata) const;
//@}

//@{
/**
* Write an image to a regular FITS file.
*
Expand All @@ -269,9 +283,16 @@ class Image : public ImageBase<PixelT> {
*/
void writeFits(std::string const& filename, fits::ImageWriteOptions const& options,
std::string const& mode = "w",
std::shared_ptr<daf::base::PropertySet const> header = nullptr,
daf::base::PropertySet const * header = nullptr,
Mask<MaskPixel> const * mask = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(std::string const& filename, fits::ImageWriteOptions const& options,
std::string const& mode,
std::shared_ptr<daf::base::PropertySet const> header,
std::shared_ptr<Mask<MaskPixel> const> mask = nullptr) const;
//@}

//@{
/**
* Write an image to a FITS RAM file.
*
Expand All @@ -283,9 +304,16 @@ class Image : public ImageBase<PixelT> {
*/
void writeFits(fits::MemFileManager& manager, fits::ImageWriteOptions const& options,
std::string const& mode = "w",
std::shared_ptr<daf::base::PropertySet const> header = nullptr,
daf::base::PropertySet const * header = nullptr,
Mask<MaskPixel> const *mask = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::MemFileManager& manager, fits::ImageWriteOptions const& options,
std::string const& mode,
std::shared_ptr<daf::base::PropertySet const> header,
std::shared_ptr<Mask<MaskPixel> const> mask = nullptr) const;
//@}

//@{
/**
* Write an image to an open FITS file object.
*
Expand All @@ -295,8 +323,13 @@ class Image : public ImageBase<PixelT> {
* @param[in] mask Mask, for calculation of statistics.
*/
void writeFits(fits::Fits& fitsfile, fits::ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> header = nullptr,
daf::base::PropertySet const * header = nullptr,
Mask<MaskPixel> const * mask = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::Fits& fitsfile, fits::ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> header,
std::shared_ptr<Mask<MaskPixel> const> mask = nullptr) const;
//@}

/**
* Read an Image from a regular FITS file.
Expand Down Expand Up @@ -467,6 +500,7 @@ class DecoratedImage {

void swap(DecoratedImage& rhs);

//@{
/**
* Write a FITS file
*
Expand All @@ -475,10 +509,15 @@ class DecoratedImage {
* @param mode "w" to write a new file; "a" to append
*/
void writeFits(std::string const& fileName,
std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet const>(),
daf::base::PropertySet const * metadata = nullptr,
std::string const& mode = "w") const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(std::string const& fileName,
std::shared_ptr<daf::base::PropertySet const> metadata,
std::string const& mode = "w") const;
//@}

//@{
/**
* Write a FITS file
*
Expand All @@ -488,9 +527,13 @@ class DecoratedImage {
* @param[in] mode "w" to write a new file; "a" to append
*/
void writeFits(std::string const& fileName, fits::ImageWriteOptions const& options,
std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet const>(),
daf::base::PropertySet const * metadata = nullptr,
std::string const& mode = "w") const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(std::string const& fileName, fits::ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> metadata,
std::string const& mode = "w") const;
//@}

/// Return a shared_ptr to the DecoratedImage's Image
std::shared_ptr<Image<PixelT>> getImage() { return _image; }
Expand Down
49 changes: 40 additions & 9 deletions include/lsst/afw/image/Mask.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,7 @@ class Mask : public ImageBase<MaskPixelT> {
*/
bool operator()(int x, int y, int plane, CheckIndices const& check) const;

//@{
/**
* Write a mask to a regular FITS file.
*
Expand All @@ -351,10 +352,15 @@ class Mask : public ImageBase<MaskPixelT> {
* @param[in] mode "w"=Create a new file; "a"=Append a new HDU.
*/
void writeFits(std::string const& fileName,
std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet>(),
daf::base::PropertySet const * metadata = nullptr,
std::string const& mode = "w") const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(std::string const& fileName,
std::shared_ptr<daf::base::PropertySet const> metadata,
std::string const& mode = "w") const;
//@}

//@{
/**
* Write a mask to a FITS RAM file.
*
Expand All @@ -363,19 +369,28 @@ class Mask : public ImageBase<MaskPixelT> {
* @param[in] mode "w"=Create a new file; "a"=Append a new HDU.
*/
void writeFits(fits::MemFileManager& manager,
std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet>(),
daf::base::PropertySet const * metadata = nullptr,
std::string const& mode = "w") const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::MemFileManager& manager,
std::shared_ptr<daf::base::PropertySet const> metadata,
std::string const& mode = "w") const;
//@}

//@{
/**
* Write a mask to an open FITS file object.
*
* @param[in] fitsfile A FITS file already open to the desired HDU.
* @param[in] metadata Additional values to write to the header (may be null).
*/
void writeFits(fits::Fits& fitsfile, std::shared_ptr<lsst::daf::base::PropertySet const> metadata =
std::shared_ptr<lsst::daf::base::PropertySet const>()) const;
void writeFits(fits::Fits& fitsfile, daf::base::PropertySet const * metadata = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::Fits& fitsfile,
std::shared_ptr<daf::base::PropertySet const> metadata) const;
//@}

//@{
/**
* Write a mask to a regular FITS file.
*
Expand All @@ -386,8 +401,14 @@ class Mask : public ImageBase<MaskPixelT> {
*/
void writeFits(std::string const& filename, fits::ImageWriteOptions const& options,
std::string const& mode = "w",
std::shared_ptr<daf::base::PropertySet const> header = nullptr) const;
daf::base::PropertySet const * header = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(std::string const& filename, fits::ImageWriteOptions const& options,
std::string const& mode,
std::shared_ptr<daf::base::PropertySet const> header) const;
//@}

//@{
/**
* Write a mask to a FITS RAM file.
*
Expand All @@ -398,8 +419,14 @@ class Mask : public ImageBase<MaskPixelT> {
*/
void writeFits(fits::MemFileManager& manager, fits::ImageWriteOptions const& options,
std::string const& mode = "w",
std::shared_ptr<daf::base::PropertySet const> header = nullptr) const;
daf::base::PropertySet const * header = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::MemFileManager& manager, fits::ImageWriteOptions const& options,
std::string const& mode,
std::shared_ptr<daf::base::PropertySet const> header) const;
//@}

//@{
/**
* Write a mask to an open FITS file object.
*
Expand All @@ -408,7 +435,11 @@ class Mask : public ImageBase<MaskPixelT> {
* @param[in] header Additional values to write to the header (may be null).
*/
void writeFits(fits::Fits& fitsfile, fits::ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> header = nullptr) const;
daf::base::PropertySet const * header = nullptr) const;
[[deprecated("Replaced by a non-shared_ptr overload. Will be removed after v24.")]]
void writeFits(fits::Fits& fitsfile, fits::ImageWriteOptions const& options,
std::shared_ptr<daf::base::PropertySet const> header) const;
//@}

/**
* Read a Mask from a regular FITS file.
Expand Down
21 changes: 19 additions & 2 deletions python/lsst/afw/fits/_fits.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,18 @@ void declareImageCompression(lsst::utils::python::WrapperCollection &wrappers) {

template <typename T>
void declareImageScalingOptionsTemplates(py::class_<ImageScalingOptions> &cls) {
cls.def("determine", &ImageScalingOptions::determine<T>);
cls.def(
"determine",
[](
ImageScalingOptions const & self,
image::ImageBase<T> const& image,
image::Mask<image::MaskPixel> const * mask
) {
return self.determine(image, mask);
},
"image"_a,
"mask"_a=nullptr
);
}

void declareImageScalingOptions(lsst::utils::python::WrapperCollection &wrappers) {
Expand Down Expand Up @@ -233,7 +244,13 @@ void declareFitsModule(lsst::utils::python::WrapperCollection &wrappers) {
},
"hdu"_a = DEFAULT_HDU, "strip"_a = false);
mod.attr("DEFAULT_HDU") = DEFAULT_HDU;
mod.def("combineMetadata", combineMetadata, "first"_a, "second"_a);
mod.def(
"combineMetadata",
py::overload_cast<daf::base::PropertyList const&, daf::base::PropertyList const &>(
combineMetadata
),
"first"_a, "second"_a
);
mod.def("makeLimitedFitsHeader", &makeLimitedFitsHeader, "metadata"_a,
"excludeNames"_a = std::set<std::string>());
mod.def(
Expand Down

0 comments on commit 9715b4d

Please sign in to comment.