Skip to content

Setup of iteratedata1d#1632

Merged
Danbr4d merged 10 commits into
developfrom
1630-iteratedata1d
Aug 29, 2023
Merged

Setup of iteratedata1d#1632
Danbr4d merged 10 commits into
developfrom
1630-iteratedata1d

Conversation

@Danbr4d
Copy link
Copy Markdown
Contributor

@Danbr4d Danbr4d commented Aug 11, 2023

Closes #1630 . Sometimes there is a need for an "intermediate" processing of Collected data in a Procedure - i.e. the collected data does not represent the final result, and must be further and non-trivially transformed in some way first.

To achieve that we should introduce something akin to an IterateData1DProcedureNode which, for now, can take a Collect1D or IntegerCollect1D node (in the same way that Process1D can). The ForEach loop of the new node will iterate over points in the (averaged) Data1D which those nodes provide, setting the values into parameters for use by other nodes.

Note that we need to follow the convention of the DataND classes when it comes to variables: one-dimensional data comprises an "x" axis with each point having an associated "value" (not "y").

@Danbr4d Danbr4d requested a review from trisyoungs August 15, 2023 15:04
Copy link
Copy Markdown
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

I think this is exactly what we're after. Just small comments and suggestions again, and you also need to create the parameter variables in the setupKeywords() function. You also need to reimplement the setName() virtual since otherwise your parameters won't get renamed if the node does (see srrc/procedure/nodes/select.cpp, line 61 for example).

Comment thread src/procedure/nodes/iteratedata1d.cpp Outdated
Comment thread src/procedure/nodes/iteratedata1d.cpp
Comment thread src/procedure/nodes/iteratedata1d.cpp Outdated
Comment thread src/procedure/nodes/iteratedata1d.h Outdated
Comment thread src/procedure/nodes/iteratedata1d.cpp Outdated
Comment thread src/procedure/nodes/iteratedata1d.cpp Outdated
Comment thread src/procedure/nodes/iteratedata1d.cpp
@Danbr4d Danbr4d force-pushed the 1630-iteratedata1d branch from 5f69c88 to 8e9c9c7 Compare August 17, 2023 11:51
@Danbr4d Danbr4d requested a review from rprospero August 22, 2023 11:37
Copy link
Copy Markdown
Contributor

@rprospero rprospero left a comment

Choose a reason for hiding this comment

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

I'm approving this, but I've also included an improvement in the comments. You can make those changes now or you can write an issue for it and we'll merge what we have.

Comment on lines +43 to +44
std::shared_ptr<const Collect1DProcedureNode> sourceData_;
std::shared_ptr<const IntegerCollect1DProcedureNode> sourceIntegerData_;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could make an abstract class AbstractCollect1DProcedureNode and have both Collect1DProcedureNode and IntegerCollect1DProcedureNode descend from it. This would allow IterateData1D to have a single sourceData_ pointer that could reference either type, depending on which concrete class it's pointing toward. This would simplify a bunch of the logic in the prepare and execute functions.

Copy link
Copy Markdown
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

Sorry - also just noticed that the new files don't conform to lowerCamelCase - the source files should be iterateData1D.h and iterateData1D.cpp.

Copy link
Copy Markdown
Member

@trisyoungs trisyoungs left a comment

Choose a reason for hiding this comment

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

👍

Comment thread src/procedure/nodes/iteratedata1d.cpp Outdated
@Danbr4d Danbr4d merged commit 85744a1 into develop Aug 29, 2023
@Danbr4d Danbr4d deleted the 1630-iteratedata1d branch August 29, 2023 13:37
rprospero pushed a commit that referenced this pull request Mar 11, 2024
rprospero pushed a commit that referenced this pull request Apr 8, 2024
rprospero pushed a commit that referenced this pull request Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Iterating over collected data

3 participants