Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DM-33470: Remove unused daf_persistence dependency #628

Merged
merged 3 commits into from
Feb 1, 2022
Merged

DM-33470: Remove unused daf_persistence dependency #628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62ddec5
Remove unused daf_persistence import from test
timj Jan 31, 2022
0f5f1e7
Remove unused code from formatters/Utils.cc
timj Jan 31, 2022
ba43998
Remove daf_persistence dependency from table file
timj Jan 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 0 additions & 37 deletions include/lsst/afw/formatters/Utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,47 +39,10 @@

#include "ndarray.h"

#include "lsst/base.h"
#include "lsst/daf/base.h"

namespace lsst {
namespace daf {
namespace base {
class PropertySet;
}
namespace persistence {
class LogicalLocation;
}
} // namespace daf
namespace afw {
namespace formatters {

/**
* Returns `true` if and only if `properties` is non-null and contains a
* unique property with the given name that has type `bool` and a value of `true`.
*/
bool extractOptionalFlag(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties,
std::string const& name);

/**
* Extracts and returns the string-valued `"itemName"` property from the given data property object.
*
* @throws lsst::pex::exceptions::InvalidParameterError
* If the given pointer is null, or the `PropertySet` pointed
* to does not contain a unique property named `"itemName"`.
*/
std::string const getItemName(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);

int extractSliceId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);
int64_t extractFpaExposureId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);
int64_t extractCcdExposureId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);
int64_t extractAmpExposureId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);
int extractVisitId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);
int extractCcdId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);
int extractAmpId(std::shared_ptr<lsst::daf::base::PropertySet const> const& properties);

int countFitsHeaderCards(lsst::daf::base::PropertySet const& prop);

/**
* Encode a std::string as a vector of uint8
*/
Expand Down
118 changes: 0 additions & 118 deletions src/formatters/Utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,132 +32,14 @@
#include <cstdint>
#include <iostream>

#include "boost/format.hpp"
#include "lsst/pex/exceptions.h"
#include "lsst/daf/base/PropertySet.h"
#include "lsst/daf/persistence/LogicalLocation.h"
#include "lsst/afw/formatters/Utils.h"

using std::int64_t;
namespace ex = lsst::pex::exceptions;
using lsst::daf::base::PropertySet;
using lsst::daf::persistence::LogicalLocation;

namespace lsst {
namespace afw {
namespace formatters {

int extractSliceId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("sliceId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"sliceId\" property has multiple values");
}
int sliceId = properties->getAsInt("sliceId");
if (sliceId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"sliceId\"");
}
if (properties->exists("universeSize") && !properties->isArray("universeSize")) {
int universeSize = properties->getAsInt("universeSize");
if (sliceId >= universeSize) {
throw LSST_EXCEPT(ex::RangeError, "\"sliceId\" must be less than \"universeSize \"");
}
}
return sliceId;
}

int extractVisitId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("visitId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"visitId\" property has multiple values");
}
int visitId = properties->getAsInt("visitId");
if (visitId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"visitId\"");
}
return visitId;
}

int64_t extractFpaExposureId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("fpaExposureId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"fpaExposureId\" property has multiple values");
}
int64_t fpaExposureId = properties->getAsInt64("fpaExposureId");
if (fpaExposureId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"fpaExposureId\"");
}
if ((fpaExposureId & 0xfffffffe00000000LL) != 0LL) {
throw LSST_EXCEPT(ex::RangeError, "\"fpaExposureId\" is too large");
}
return fpaExposureId;
}

int extractCcdId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("ccdId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"ccdId\" property has multiple values");
}
int ccdId = properties->getAsInt("ccdId");
if (ccdId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"ccdId\"");
}
if (ccdId > 255) {
throw LSST_EXCEPT(ex::RangeError, "\"ccdId\" is too large");
}
return static_cast<int>(ccdId);
}

int extractAmpId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("ampId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"ampId\" property has multiple values");
}
int ampId = properties->getAsInt("ampId");
if (ampId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"ampId\"");
}
if (ampId > 63) {
throw LSST_EXCEPT(ex::RangeError, "\"ampId\" is too large");
}
return (extractCcdId(properties) << 6) + ampId;
}

int64_t extractCcdExposureId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("ccdExposureId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"ccdExposureId\" property has multiple values");
}
int64_t ccdExposureId = properties->getAsInt64("ccdExposureId");
if (ccdExposureId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"ccdExposureId\"");
}
return ccdExposureId;
}

int64_t extractAmpExposureId(std::shared_ptr<PropertySet const> const& properties) {
if (properties->isArray("ampExposureId")) {
throw LSST_EXCEPT(ex::RuntimeError, "\"ampExposureId\" property has multiple values");
}
int64_t ampExposureId = properties->getAsInt64("ampExposureId");
if (ampExposureId < 0) {
throw LSST_EXCEPT(ex::RangeError, "negative \"ampExposureId\"");
}
return ampExposureId;
}

std::string const getItemName(std::shared_ptr<PropertySet const> const& properties) {
if (!properties) {
throw LSST_EXCEPT(ex::InvalidParameterError, "Null std::shared_ptr<PropertySet>");
}
if (properties->isArray("itemName")) {
throw LSST_EXCEPT(ex::InvalidParameterError, "\"itemName\" property has multiple values");
}
return properties->getAsString("itemName");
}

bool extractOptionalFlag(std::shared_ptr<PropertySet const> const& properties, std::string const& name) {
if (properties && properties->exists(name)) {
return properties->getAsBool(name);
}
return false;
}

int countFitsHeaderCards(lsst::daf::base::PropertySet const& prop) { return prop.paramNames(false).size(); }

ndarray::Array<std::uint8_t, 1, 1> stringToBytes(std::string const& str) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obvious question is whether to move what's left of this file into another location because it's not really anything to do with formatters any more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the place to move it would be somewhere under afw.table. It's not really as connected with typehandling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that these two functions are only used for tables so I suppose I could try to move them somewhere. Unless you have a good idea where I'm inclined to merge and get the easy win of daf_persistence avoidance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. If you've removed them, but they are still "used for tables", why isn't something breaking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are talking about the two functions that are left in the file. They seem to be used by WCS for example to convert to bytes to stuff into a table and then convert back to strings to read out of a table.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok. I'd lean towards moving them into the source files that use them, if it's just a couple of source files (not headers), but if it's much work at all, then leaving them where they are is probably best, since reducing churn is good, too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. There are four places:

include/lsst/afw/typehandling/python.h
178:        std::string stringRep = formatters::bytesToString(record.get(keys.bytes));
202:    record->set(keys.bytes, formatters::stringToBytes(o));

src/math/TransformBoundedField.cc
112:        auto frameSetStr = formatters::bytesToString(record.get(keys.frameSet));
137:    record->set(keys.frameSet, formatters::stringToBytes(getTransform().writeString()));

src/geom/SkyWcs.cc
98:        std::string stringRep = formatters::bytesToString(record.get(keys.wcs));
385:    record->set(keys.wcs, formatters::stringToBytes(writeString()));

src/geom/Transform.cc
232:        std::string stringRep = formatters::bytesToString(record.get(keys.bytes));
244:    record->set(keys.bytes, formatters::stringToBytes(writeString()));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'll merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the right thing to do might have been to look at the common usage in the above four locations. It's always getting or setting in a BaseRecord, so that might be the place to move it.

auto nbytes = str.size() * sizeof(char) / sizeof(std::uint8_t);
std::uint8_t const* byteCArr = reinterpret_cast<std::uint8_t const*>(str.data());
Expand Down
1 change: 0 additions & 1 deletion tests/test_fitsCompression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@

import lsst.utils
import lsst.daf.base
import lsst.daf.persistence
import lsst.geom
import lsst.afw.geom
import lsst.afw.image
Expand Down
2 changes: 1 addition & 1 deletion ups/afw.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import lsst.sconsUtils

dependencies = {
"required": ["cpputils", "daf_persistence", "daf_base", "pex_exceptions", "geom", "log", "pex_config",
"required": ["cpputils", "daf_base", "pex_exceptions", "geom", "log", "pex_config",
"eigen", "fftw", "ndarray", "numpy", "minuit2", "gsl", "cfitsio",
"astshim", "sphgeom"],
"buildRequired": ["boost_test", "boost_timer", "pybind11"],
Expand Down
1 change: 0 additions & 1 deletion ups/afw.table
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
setupRequired(daf_base)
setupRequired(daf_persistence)
setupRequired(pex_config)
setupRequired(geom)
setupRequired(log)
Expand Down