Skip to content

Commit

Permalink
ENH: Add Additional Warnings When Moving/Copying DataObjects with a T…
Browse files Browse the repository at this point in the history
…uple Mismatch (BlueQuartzSoftware#804)
  • Loading branch information
nyoungbq authored Dec 26, 2023
1 parent ca4c818 commit 6b7b1f0
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "CopyDataObjectFilter.hpp"

#include "simplnx/DataStructure/AttributeMatrix.hpp"
#include "simplnx/DataStructure/BaseGroup.hpp"
#include "simplnx/Filter/Actions/CopyDataObjectAction.hpp"
#include "simplnx/Parameters/BoolParameter.hpp"
Expand Down Expand Up @@ -80,14 +81,35 @@ IFilter::PreflightResult CopyDataObjectFilter::preflightImpl(const DataStructure
return {MakeErrorResult<OutputActions>(-27360, "Copied Object(s) have the same parent as original data but the suffix is empty."), {}};
}

OutputActions actions;
Result<OutputActions> resultOutputActions;

for(const auto& dataArrayPath : dataArrayPaths)
{
DataPath parentPath = dataArrayPath.getParent();
if(useNewParent)
{
parentPath = args.value<DataPath>(k_NewPath_Key);
// Scope AM check since we fully expect it to be a nullptr
{
const auto* possibleAM = data.getDataAs<AttributeMatrix>(parentPath);
if(possibleAM != nullptr)
{
for(const auto& path : dataArrayPaths)
{
const auto* possibleIArray = data.getDataAs<IArray>(path);
if(possibleIArray != nullptr)
{
if(possibleAM->getShape() != possibleIArray->getTupleShape())
{
resultOutputActions.warnings().push_back(
Warning{-27361, fmt::format("The tuple dimensions of {} [{}] do not match the AttributeMatrix {} tuple dimensions [{}]. This could result in a runtime error if the sizing remains "
"the same at time of this filters execution. Proceed with caution.",
possibleIArray->getName(), possibleIArray->getNumberOfTuples(), possibleAM->getName(), possibleAM->getNumTuples())});
}
}
}
}
}
}
std::string newTargetName = dataArrayPath.getTargetName() + suffix;
DataPath newDataPath = parentPath.createChildPath(newTargetName);
Expand All @@ -100,16 +122,16 @@ IFilter::PreflightResult CopyDataObjectFilter::preflightImpl(const DataStructure
{
for(const auto& sourcePath : pathsToBeCopied.value())
{
std::string createdPathName = nx::core::StringUtilities::replace(sourcePath.toString(), dataArrayPath.getTargetName(), newTargetName);
std::string createdPathName = StringUtilities::replace(sourcePath.toString(), dataArrayPath.getTargetName(), newTargetName);
allCreatedPaths.push_back(DataPath::FromString(createdPathName).value());
}
}
}
auto action = std::make_unique<CopyDataObjectAction>(dataArrayPath, newDataPath, allCreatedPaths);
actions.appendAction(std::move(action));
resultOutputActions.value().appendAction(std::move(action));
}

return {std::move(actions)};
return {std::move(resultOutputActions)};
}

//------------------------------------------------------------------------------
Expand Down
29 changes: 22 additions & 7 deletions src/Plugins/SimplnxCore/src/SimplnxCore/Filters/MoveData.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "MoveData.hpp"

#include "simplnx/DataStructure/AttributeMatrix.hpp"
#include "simplnx/DataStructure/BaseGroup.hpp"
#include "simplnx/Filter/Actions/MoveDataAction.hpp"
#include "simplnx/Parameters/DataGroupSelectionParameter.hpp"
Expand All @@ -10,13 +11,6 @@

namespace nx::core
{
namespace
{
constexpr int64 k_TupleCountInvalidError = -250;
constexpr int64 k_MissingFeaturePhasesError = -251;

} // namespace

//------------------------------------------------------------------------------
std::string MoveData::name() const
{
Expand Down Expand Up @@ -74,6 +68,27 @@ IFilter::PreflightResult MoveData::preflightImpl(const DataStructure& data, cons
Result<OutputActions> resultOutputActions;
std::vector<PreflightValue> preflightUpdatedValues;

// Scope AM check since we fully expect it to be a nullptr
{
const auto* possibleAM = data.getDataAs<AttributeMatrix>(newParentPath);
if(possibleAM != nullptr)
{
for(const auto& path : dataPaths)
{
const auto* possibleIArray = data.getDataAs<IArray>(path);
if(possibleIArray != nullptr)
{
if(possibleAM->getShape() != possibleIArray->getTupleShape())
{
resultOutputActions.warnings().push_back(Warning{-69250, fmt::format("The tuple dimensions of {} [{}] do not match the AttributeMatrix {} tuple dimensions [{}]. This could result in a "
"runtime error if the sizing remains the same at time of this filters execution. Proceed with caution.",
possibleIArray->getName(), possibleIArray->getNumberOfTuples(), possibleAM->getName(), possibleAM->getNumTuples())});
}
}
}
}
}

for(const auto& dataPath : dataPaths)
{
auto moveDataAction = std::make_unique<MoveDataAction>(dataPath, newParentPath);
Expand Down
21 changes: 21 additions & 0 deletions src/Plugins/SimplnxCore/test/CopyDataObjectTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@

using namespace nx::core;

namespace
{
const int32 k_TupleDimMismatchWarningCode = -27361;
}

TEST_CASE("SimplnxCore::CopyDataObjectFilter(Valid Execution)", "[SimplnxCore][CopyDataObjectFilter]")
{
static const DataPath k_DataPath1({Constants::k_SmallIN100, "Phase Data"});
Expand Down Expand Up @@ -95,6 +100,22 @@ TEST_CASE("SimplnxCore::CopyDataObjectFilter(Invalid Parameters)", "[SimplnxCore
args.insert(CopyDataObjectFilter::k_NewPath_Key, std::make_any<DataPath>(copyPath));
args.insert(CopyDataObjectFilter::k_NewPathSuffix_Key, std::make_any<std::string>("_COPY"));

// Preflight the filter and check result
auto preflightResult = filter.preflight(dataStructure, args);

REQUIRE_FALSE(preflightResult.outputActions.warnings().empty());

bool found = false;
for(const auto& warning : preflightResult.outputActions.warnings())
{
if(warning.code == ::k_TupleDimMismatchWarningCode)
{
found = true;
}
}

REQUIRE(found);

auto result = filter.execute(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(result.result);
}
Expand Down
56 changes: 55 additions & 1 deletion src/Plugins/SimplnxCore/test/MoveDataTest.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "SimplnxCore/Filters/MoveData.hpp"

#include "simplnx/DataStructure/AttributeMatrix.hpp"
#include "simplnx/DataStructure/DataArray.hpp"
#include "simplnx/DataStructure/DataGroup.hpp"
#include "simplnx/UnitTest/UnitTestCommon.hpp"

Expand All @@ -16,6 +18,11 @@ constexpr StringLiteral k_Group2Name = "Group2";
constexpr StringLiteral k_Group3Name = "Group3";
constexpr StringLiteral k_Group4Name = "Group4";

const int32 k_TupleDimMismatchWarningCode = -69250;

constexpr StringLiteral k_AM1Name = "Matrix1";
constexpr StringLiteral k_DataArray1Name = "Array1";

DataStructure createDataStructure()
{
DataStructure data;
Expand All @@ -24,6 +31,11 @@ DataStructure createDataStructure()
auto group3 = DataGroup::Create(data, k_Group3Name, group2->getId());
auto group4 = DataGroup::Create(data, k_Group4Name, group2->getId());

auto am1 = AttributeMatrix::Create(data, k_AM1Name, std::vector<usize>{2});

auto dataStore = std::make_unique<DataStore<uint8>>(std::vector<usize>{20}, std::vector<usize>{3}, 0);
auto dataArray1 = DataArray<uint8>::Create(data, k_DataArray1Name, std::move(dataStore));

data.setAdditionalParent(group4->getId(), group3->getId());
return data;
}
Expand Down Expand Up @@ -80,19 +92,61 @@ TEST_CASE("SimplnxCore::MoveData Unsuccessful", "[Complex::Core][MoveData]")
{
args.insertOrAssign(MoveData::k_Data_Key, std::make_any<std::vector<DataPath>>({k_Group3Path}));
args.insertOrAssign(MoveData::k_NewParent_Key, std::make_any<DataPath>(k_Group2Path));

// Preflight the filter and check result
auto preflightResult = filter.preflight(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(preflightResult.outputActions);
}
SECTION("Cannot reparent object to itself")
{
args.insertOrAssign(MoveData::k_Data_Key, std::make_any<std::vector<DataPath>>({k_Group3Path}));
args.insertOrAssign(MoveData::k_NewParent_Key, std::make_any<DataPath>(k_Group3Path));

// Preflight the filter and check result
auto preflightResult = filter.preflight(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(preflightResult.outputActions);
}
SECTION("Cannot reparent object to a child object")
{
args.insertOrAssign(MoveData::k_Data_Key, std::make_any<std::vector<DataPath>>({k_Group2Path}));
args.insertOrAssign(MoveData::k_NewParent_Key, std::make_any<DataPath>(k_Group3Path));

// Preflight the filter and check result
auto preflightResult = filter.preflight(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(preflightResult.outputActions);
}
}

TEST_CASE("SimplnxCore::MoveData Tuple Size Mismatches Warning and Failure", "[Complex::Core][MoveData]")
{
MoveData filter;
Arguments args;
DataStructure dataStructure = createDataStructure();

const DataPath k_DataArrayPath({k_DataArray1Name});
const DataPath k_AMPath({k_AM1Name});

args.insertOrAssign(MoveData::k_Data_Key, std::make_any<std::vector<DataPath>>({k_DataArrayPath}));
args.insertOrAssign(MoveData::k_NewParent_Key, std::make_any<DataPath>(k_AMPath));

// Preflight the filter and check result
auto preflightResult = filter.preflight(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(preflightResult.outputActions);

REQUIRE_FALSE(preflightResult.outputActions.warnings().empty());

bool found = false;
for(const auto& warning : preflightResult.outputActions.warnings())
{
if(warning.code == ::k_TupleDimMismatchWarningCode)
{
found = true;
}
}

REQUIRE(found);

SIMPLNX_RESULT_REQUIRE_VALID(preflightResult.outputActions);

auto result = filter.execute(dataStructure, args);
SIMPLNX_RESULT_REQUIRE_INVALID(result.result);
}
1 change: 0 additions & 1 deletion src/simplnx/Filter/Actions/MoveDataAction.cpp
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
#include "MoveDataAction.hpp"

#include "simplnx/DataStructure/BaseGroup.hpp"
#include "simplnx/Utilities/DataArrayUtilities.hpp"

#include <fmt/core.h>

Expand Down

0 comments on commit 6b7b1f0

Please sign in to comment.