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

FluidGrid: proposed dimensions output to the fitTransform method #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

FluidGrid: proposed dimensions output to the fitTransform method #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a1e4df
FluidGrid: proposed dimensions output to the fitTransform method
tremblap Sep 19, 2021
9679e75
Update include/algorithms/public/Grid.hpp
tremblap Sep 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion include/algorithms/public/Grid.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ class Grid
double area = (xMax - xMin) * (yMax - yMin);
double size = static_cast<double>(N);
double step = sqrt(area / M);
index numCols, numRows;

if (area <= 0) return DataSet();

Expand Down Expand Up @@ -100,8 +99,14 @@ class Grid
return result;
}

std::pair<index, index> const getGridMax() {
return {numCols,numRows};
}

private:
Assign2D assign2D;
index numCols{0};
indes numRows{0};
};
}; // namespace algorithm
}; // namespace fluid
12 changes: 7 additions & 5 deletions include/clients/nrt/GridClient.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ class GridClient : public FluidBaseClient, OfflineIn, OfflineOut, ModelObject
using string = std::string;
using BufferPtr = std::shared_ptr<BufferAdaptor>;
using StringVector = FluidTensor<string, 1>;
using IndexPair = std::tuple<index,index>;
Copy link
Member

Choose a reason for hiding this comment

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

Note to other reviewers: this needs to be std::tuple here because the CCE wrappers can handle that as a message return, but not (yet) std::pair. If / when the wrappers are updated to account for pair, then this slight inconsistency w/r/t the algorithm could be ironed out

Copy link
Member Author

Choose a reason for hiding this comment

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

Credits should be given: with @weefuzzy I would have never been able to do this and his patience was infinite as usual.

Copy link
Member Author

Choose a reason for hiding this comment

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

another question, meta, to all reviewer: did I get the dependencies right? I put the PR here in core, and did only feature branches in the two CCE wrapper repos. I think it is clear enough and doesn't clutter the PR of the other 2 but I know @weefuzzy did differently for another one (the new rolling stat object).


template <typename T>
Result process(FluidContext&)
Expand All @@ -56,21 +57,22 @@ class GridClient : public FluidBaseClient, OfflineIn, OfflineOut, ModelObject

GridClient(ParamSetViewType& p) : mParams(p) {}

MessageResult<void> fitTransform(DataSetClientRef sourceClient,
MessageResult<IndexPair> fitTransform(DataSetClientRef sourceClient,
DataSetClientRef destClient)
{
auto srcPtr = sourceClient.get().lock();
auto destPtr = destClient.get().lock();
if (!srcPtr || !destPtr) return Error(NoDataSet);
if (!srcPtr || !destPtr) return Error<IndexPair>(NoDataSet);
auto src = srcPtr->getDataSet();
auto dest = destPtr->getDataSet();
if (src.dims() != 2) return Error("Dataset should be 2D");
if (src.size() == 0) return Error(EmptyDataSet);
if (src.dims() != 2) return Error<IndexPair>("Dataset should be 2D");
if (src.size() == 0) return Error<IndexPair>(EmptyDataSet);
FluidDataSet<string, double, 1> result;
Copy link
Member

Choose a reason for hiding this comment

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

result doesn't make as much sense here now that there is actually a return value from the message. Also declaring and assigning like this means that we miss an oppurtunity for Return Value Optimisation (i.e the result of Grid::process() gets constructed directly into the FluidTensor in this scope). Would suggest

Suggested change
FluidDataSet<string, double, 1> result;
FluidDataSet<string,double, 1> destination = mAlgorithm.process(...

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 like that as it frees result for the output indeed.

result = mAlgorithm.process(src,
get<kResample>(), get<kExtent>(), get<kAxis>());
destPtr->setDataSet(result);
return OK();
IndexPair casted = mAlgorithm.getGridMax();
Copy link
Member

Choose a reason for hiding this comment

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

and I would then call this result (this isn't really a cast, it happens to be a conversion). Did you try returning directly without the intervening temporary tuple?

Suggested change
IndexPair casted = mAlgorithm.getGridMax();
IndexPair result(mAlgorithm.getGridMax());
return {result};

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 tried without the tuple and it moaned. I can retry now that everything else is working.

return {casted};
}

static auto getMessageDescriptors()
Expand Down