Skip to content

Commit

Permalink
Fix: Strings with a single char in Python (#1585)
Browse files Browse the repository at this point in the history 
* Better conversion for string and char type attributes

* Err to the side of "string in doubt"

Instead of "char in doubt"

* Avoid throwing in tryCast
  • Loading branch information
@franzpoeschel
franzpoeschel authored Jan 17, 2024
1 parent d5524ec commit 3a8a220
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 31 deletions.
24 changes: 24 additions & 0 deletions include/openPMD/auxiliary/TypeTraits.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,27 @@ namespace detail
constexpr static bool value = true;
using type = T;
};

template <typename>
struct IsChar
{
constexpr static bool value = false;
};
template <>
struct IsChar<char>
{
constexpr static bool value = true;
};
template <>
struct IsChar<signed char>
{
constexpr static bool value = true;
};
template <>
struct IsChar<unsigned char>
{
constexpr static bool value = true;
};
} // namespace detail

template <typename T>
Expand All @@ -117,6 +138,9 @@ inline constexpr bool IsPointer_v = detail::IsPointer<T>::value;
template <typename T>
using IsPointer_t = typename detail::IsPointer<T>::type;

template <typename C>
inline constexpr bool IsChar_v = detail::IsChar<C>::value;

/** Emulate in the C++ concept ContiguousContainer
*
* Users can implement this trait for a type to signal it can be used as
Expand Down
129 changes: 113 additions & 16 deletions include/openPMD/backend/Attribute.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,60 +106,121 @@ class Attribute : public auxiliary::Variant<Datatype, attribute_types>
namespace detail
{
template <typename T, typename U>
auto doConvert(T *pv) -> std::variant<U, std::runtime_error>
auto doConvert(T const *pv) -> std::variant<U, std::runtime_error>
{
(void)pv;
if constexpr (std::is_convertible_v<T, U>)
{
return {static_cast<U>(*pv)};
}
else if constexpr (
std::is_same_v<T, std::string> && auxiliary::IsChar_v<U>)
{
if (pv->size() == 1)
{
return static_cast<U>(pv->at(0));
}
else
{
return {
std::runtime_error("getCast: cast from string to char only "
"possible if string has length 1.")};
}
}
else if constexpr (
auxiliary::IsChar_v<T> && std::is_same_v<U, std::string>)
{
return std::string(1, *pv);
}
else if constexpr (auxiliary::IsVector_v<T> && auxiliary::IsVector_v<U>)
{
U res{};
res.reserve(pv->size());
if constexpr (std::is_convertible_v<
typename T::value_type,
typename U::value_type>)
{
U res{};
res.reserve(pv->size());
std::copy(pv->begin(), pv->end(), std::back_inserter(res));
return {res};
}
else
{
return {
std::runtime_error("getCast: no vector cast possible.")};
// try a dynamic conversion recursively
for (auto const &val : *pv)
{
auto conv = doConvert<
typename T::value_type,
typename U::value_type>(&val);
if (auto conv_val =
std::get_if<typename U::value_type>(&conv);
conv_val)
{
res.push_back(std::move(*conv_val));
}
else
{
auto exception = std::get<std::runtime_error>(conv);
return {std::runtime_error(
std::string(
"getCast: no vector cast possible, recursive "
"error: ") +
exception.what())};
}
}
return {res};
}
}
// conversion cast: array to vector
// if a backend reports a std::array<> for something where
// the frontend expects a vector
else if constexpr (auxiliary::IsArray_v<T> && auxiliary::IsVector_v<U>)
{
U res{};
res.reserve(pv->size());
if constexpr (std::is_convertible_v<
typename T::value_type,
typename U::value_type>)
{
U res{};
res.reserve(pv->size());
std::copy(pv->begin(), pv->end(), std::back_inserter(res));
return {res};
}
else
{
return {std::runtime_error(
"getCast: no array to vector conversion possible.")};
// try a dynamic conversion recursively
for (auto const &val : *pv)
{
auto conv = doConvert<
typename T::value_type,
typename U::value_type>(&val);
if (auto conv_val =
std::get_if<typename U::value_type>(&conv);
conv_val)
{
res.push_back(std::move(*conv_val));
}
else
{
auto exception = std::get<std::runtime_error>(conv);
return {std::runtime_error(
std::string(
"getCast: no array to vector conversion "
"possible, recursive error: ") +
exception.what())};
}
}
return {res};
}
}
// conversion cast: vector to array
// if a backend reports a std::vector<> for something where
// the frontend expects an array
else if constexpr (auxiliary::IsVector_v<T> && auxiliary::IsArray_v<U>)
{
U res{};
if constexpr (std::is_convertible_v<
typename T::value_type,
typename U::value_type>)
{
U res{};
if (res.size() != pv->size())
{
return std::runtime_error(
Expand All @@ -175,24 +236,60 @@ namespace detail
}
else
{
return {std::runtime_error(
"getCast: no vector to array conversion possible.")};
// try a dynamic conversion recursively
for (size_t i = 0; i <= res.size(); ++i)
{
auto const &val = (*pv)[i];
auto conv = doConvert<
typename T::value_type,
typename U::value_type>(&val);
if (auto conv_val =
std::get_if<typename U::value_type>(&conv);
conv_val)
{
res[i] = std::move(*conv_val);
}
else
{
auto exception = std::get<std::runtime_error>(conv);
return {std::runtime_error(
std::string(
"getCast: no vector to array conversion "
"possible, recursive error: ") +
exception.what())};
}
}
return {res};
}
}
// conversion cast: turn a single value into a 1-element vector
else if constexpr (auxiliary::IsVector_v<U>)
{
U res{};
res.reserve(1);
if constexpr (std::is_convertible_v<T, typename U::value_type>)
{
U res{};
res.reserve(1);
res.push_back(static_cast<typename U::value_type>(*pv));
return {res};
}
else
{
return {std::runtime_error(
"getCast: no scalar to vector conversion possible.")};
// try a dynamic conversion recursively
auto conv = doConvert<T, typename U::value_type>(pv);
if (auto conv_val = std::get_if<typename U::value_type>(&conv);
conv_val)
{
res.push_back(std::move(*conv_val));
return {res};
}
else
{
auto exception = std::get<std::runtime_error>(conv);
return {std::runtime_error(
std::string("getCast: no scalar to vector conversion "
"possible, recursive error: ") +
exception.what())};
}
}
}
else
Expand Down
22 changes: 11 additions & 11 deletions src/Mesh.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ Mesh &Mesh::setDataOrder(Mesh::DataOrder dor)

std::vector<std::string> Mesh::axisLabels() const
{
return getAttribute("axisLabels").get<std::vector<std::string> >();
return getAttribute("axisLabels").get<std::vector<std::string>>();
}

Mesh &Mesh::setAxisLabels(std::vector<std::string> const &als)
Expand All @@ -165,7 +165,7 @@ template Mesh &Mesh::setGridSpacing(std::vector<long double> const &gs);

std::vector<double> Mesh::gridGlobalOffset() const
{
return getAttribute("gridGlobalOffset").get<std::vector<double> >();
return getAttribute("gridGlobalOffset").get<std::vector<double>>();
}

Mesh &Mesh::setGridGlobalOffset(std::vector<double> const &ggo)
Expand Down Expand Up @@ -331,9 +331,9 @@ void Mesh::read()
aRead.name = "axisLabels";
IOHandler()->enqueue(IOTask(this, aRead));
IOHandler()->flush(internal::defaultFlushParams);
if (*aRead.dtype == DT::VEC_STRING || *aRead.dtype == DT::STRING)
setAxisLabels(
Attribute(*aRead.resource).get<std::vector<std::string> >());
Attribute a = Attribute(*aRead.resource);
if (auto val = a.getOptional<std::vector<std::string>>(); val.has_value())
setAxisLabels(*val);
else
throw error::ReadError(
error::AffectedObject::Attribute,
Expand All @@ -346,16 +346,16 @@ void Mesh::read()
aRead.name = "gridSpacing";
IOHandler()->enqueue(IOTask(this, aRead));
IOHandler()->flush(internal::defaultFlushParams);
Attribute a = Attribute(*aRead.resource);
a = Attribute(*aRead.resource);
if (*aRead.dtype == DT::VEC_FLOAT || *aRead.dtype == DT::FLOAT)
setGridSpacing(a.get<std::vector<float> >());
setGridSpacing(a.get<std::vector<float>>());
else if (*aRead.dtype == DT::VEC_DOUBLE || *aRead.dtype == DT::DOUBLE)
setGridSpacing(a.get<std::vector<double> >());
setGridSpacing(a.get<std::vector<double>>());
else if (
*aRead.dtype == DT::VEC_LONG_DOUBLE || *aRead.dtype == DT::LONG_DOUBLE)
setGridSpacing(a.get<std::vector<long double> >());
setGridSpacing(a.get<std::vector<long double>>());
// conversion cast if a backend reports an integer type
else if (auto val = a.getOptional<std::vector<double> >(); val.has_value())
else if (auto val = a.getOptional<std::vector<double>>(); val.has_value())
setGridSpacing(val.value());
else
throw error::ReadError(
Expand All @@ -370,7 +370,7 @@ void Mesh::read()
IOHandler()->enqueue(IOTask(this, aRead));
IOHandler()->flush(internal::defaultFlushParams);
if (auto val =
Attribute(*aRead.resource).getOptional<std::vector<double> >();
Attribute(*aRead.resource).getOptional<std::vector<double>>();
val.has_value())
setGridGlobalOffset(val.value());
else
Expand Down
36 changes: 32 additions & 4 deletions src/binding/python/Attributable.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include <iterator>
#include <map>
#include <optional>
#include <pybind11/pytypes.h>
#include <string>
#include <type_traits>
#include <vector>
Expand Down Expand Up @@ -336,6 +337,16 @@ struct char_to_explicit_char<false>
template <typename TargetType>
std::optional<TargetType> tryCast(py::object const &obj)
{
// Do a check to avoid throwing exceptions
if constexpr (std::is_default_constructible_v<TargetType>)
{
TargetType val{};
auto python_val = py::cast(std::move(val));
if (!py::isinstance(obj, python_val.get_type()))
{
return std::nullopt;
}
}
try
{
return obj.cast<TargetType>();
Expand All @@ -358,17 +369,25 @@ bool setAttributeFromObject_char(
std::is_same_v<Char_t, char>,
typename char_to_explicit_char<>::type,
Char_t>;
using ListChar = std::vector<char>;
using ListString = std::vector<std::string>;

/*
* We cannot distinguish strings with length 1 from chars at this place,
* so avoid this cast. If the attribute is actually a char, skipping this
* branch means that it might be upcasted to string.
*/
#if 0
using ListChar = std::vector<char>;
if (auto casted_char = tryCast<char>(obj); casted_char.has_value())
{
return attr.setAttribute<char>(key, *casted_char);
}
} else
#endif

// This must come after tryCast<char>
// because tryCast<string> implicitly covers chars as well.
else if (auto casted_string = tryCast<std::string>(obj);
casted_string.has_value())
if (auto casted_string = tryCast<std::string>(obj);
casted_string.has_value())
{
return attr.setAttribute<std::string>(key, std::move(*casted_string));
}
Expand All @@ -386,11 +405,20 @@ bool setAttributeFromObject_char(
// NOW: List casts.
// All list casts must come after all scalar casts,
// because list casts implicitly cover scalars too.

/*
* We cannot distinguish strings with length 1 from chars at this place,
* so avoid this cast. If the attribute is actually a vector of char,
* skipping this branch means that it might be upcasted to a vector of
* string.
*/
#if 0
else if (auto list_of_char = tryCast<ListChar>(obj);
list_of_char.has_value())
{
return attr.setAttribute<ListChar>(key, std::move(*list_of_char));
}
#endif
// this must come after tryCast<vector<char>>,
// because tryCast<vector<string>> implicitly covers chars as well
else if (auto list_of_string = tryCast<ListString>(obj);
Expand Down

0 comments on commit 3a8a220

Please sign in to comment.