Enable two-dimensional array in the existing data structure by mayukh33 · Pull Request #51 · mphowardlab/flyft

mayukh33 · 2025-02-20T21:05:20Z

No description provided.

for more information, see https://pre-commit.ci

mphoward

Generally a good first pass! Here are some comments that need addressing / clarification.

mphoward · 2025-02-24T14:29:52Z

include/flyft/data_layout.h

    {
 class DataLayout
    {
+    //! DataLayout class generates the template for the custom data types used in the software


These are not proper doxygen comments. Please read about how you mark either short vs. long descriptions, and we can discuss if you have questions.

mphoward · 2025-02-24T14:31:12Z

src/data_layout.cc

+DataLayout::DataLayout()
+    {
+    const std::vector<int>& shape_ = {0, 0};
+    }


Wouldn't the default be an empty array now?

Suggested change

DataLayout::DataLayout()

{

const std::vector<int>& shape_ = {0, 0};

}

DataLayout::DataLayout()

{

}

mphoward · 2025-02-24T14:31:41Z

src/data_layout.cc

+    //! Strided indexing scheme for converting
+    //! N dimensional array into one dimensional array


Doxygen comments do not go in regular code, use regular comments

mphoward · 2025-02-24T14:32:40Z

src/data_layout.cc

-int DataLayout::size() const
+std::vector<int> DataLayout::size() const
    {
    return shape();


This isn't right anymore: you want to take the product of the shape_ to return the total size

mphoward · 2025-02-24T14:35:20Z

include/flyft/data_view.h

+    reference operator()(const std::vector<int>& idx) const
        {
-        return data_[layout_(start_ + idx)];
+        std::transform(idx.begin(), idx.end(), start_, idx.begin(), std::plus<int>());


What is this doing, and why can't you just pass the idx directly? It doesn't seem like this should do anything / work because idx is const.

I was trying elementwise summation of the idx vector with start_ to replicate the operation performed in the previous implementation of this function without using loops

Oh, got it! Yes, this is necessary then, but you should use a new local variable to hold the result. std::transform is essentially a loop, so I doubt it will be more performant than a loop.

mphoward · 2025-02-24T14:35:37Z

include/flyft/data_view.h

        }

    private:
+    /// @brief


That was auto-generated by accident. I will remove it

mphoward · 2025-02-28T16:32:01Z

include/flyft/data_layout.h

+//! DataLayout
+/*!
+ * Generates data structure for the custom data types used in the software.
+ * The class provides get and set functions for the array index and implements N-dimensional array
+ * mapping into one dimensional array using strided indexing scheme.
+ */


The short description should be what the object is (the class name will be part of what doxygen generates), then the long description should describe what the class does (potentially with examples longer term)

Suggested change

//! DataLayout

/*!

* Generates data structure for the custom data types used in the software.

* The class provides get and set functions for the array index and implements N-dimensional array

* mapping into one dimensional array using strided indexing scheme.

*/

//! Multidimensional array layout

/*!

* DataLayout maps an N-dimensional index to one-dimensional

* index using row-major ordering. Negative indexes are supported

* and interpreted as relative to the zero-index.

*/

mphoward · 2025-02-28T16:33:07Z

include/flyft/data_layout.h

+    //! Constructor for the DataLayout
+    /*!
+     * \param shape_ Shape of the index array
+     */


Don't include class name in short descriptions. Use periods to end parameter descriptions. Parameter name needs to match argument name.

Suggested change

//! Constructor for the DataLayout

/*!

* \param shape_ Shape of the index array

*/

//! Constructor

/*!

* \param shape Number of elements in array along each dimension.

*/

mphoward · 2025-02-28T16:34:09Z

include/flyft/data_layout.h

+    // Getter for the class
+    int operator()(const std::vector<int>& idx) const;


This is not a doxygen comment and not descriptive

Suggested change

// Getter for the class

int operator()(const std::vector<int>& idx) const;

//! Map a multidimensional index to a one-dimensional index

/*!

* \param idx Multidimensional index.

* \return One-dimensional index.

*

* Row-major ordering is used.

*/

int operator()(const std::vector<int>& idx) const;

mphoward · 2025-02-28T16:34:36Z

include/flyft/data_layout.h

+    // Setter for shape
+    std::vector<int> shape() const;


This is not doxygen and it is also not a setter. It's OK, though, to only use a short description for getters.

Suggested change

// Setter for shape

std::vector<int> shape() const;

//! Shape of the layout in each dimension

std::vector<int> shape() const;

mphoward · 2025-02-28T16:36:08Z

include/flyft/data_layout.h

+
+    // Setter for shape
+    std::vector<int> shape() const;
+    // Getter for shape


Suggested change

// Getter for shape

//! Total number of elements in the layout

mphoward · 2025-02-28T16:37:16Z

include/flyft/data_layout.h

    bool operator==(const DataLayout& other) const;
    bool operator!=(const DataLayout& other) const;


Suggested change

bool operator==(const DataLayout& other) const;

bool operator!=(const DataLayout& other) const;

//! Test if two layouts are equal

/*!

* \return True if the layouts have the same shape.

*/

bool operator==(const DataLayout& other) const;

//! Test if two layouts are not equal

bool operator!=(const DataLayout& other) const;

mphoward · 2025-02-28T16:37:38Z

include/flyft/data_layout.h


    private:
-    int shape_;
+    std::vector<int> shape_;


Suggested change

std::vector<int> shape_;

std::vector<int> shape_; //!< Multidimensional shape of layout

include/flyft/data_view.h

mphoward · 2025-02-28T16:53:29Z

src/data_layout.cc

    {

-DataLayout::DataLayout() : shape_(0) {}
+// Default constructor


I tend to like putting all the doxygen comments in the header. They only need to be in one spot.

mphoward · 2025-03-06T17:24:22Z

include/flyft/data_layout.h

    {
+//! Multidimensional array layout
+/*!
+ * DataLayout maps an N-dimensional index to one-dimensional


Suggested change

* DataLayout maps an N-dimensional index to one-dimensional

* DataLayout maps an N-dimensional index to a one-dimensional

mphoward · 2025-03-06T17:24:28Z

include/flyft/data_layout.h

    public:
    DataLayout();
-    explicit DataLayout(int shape_);
+    //! Constructor for the DataLayout


Suggested change

//! Constructor for the DataLayout

//! Constructor

mphoward · 2025-03-06T17:24:35Z

include/flyft/data_layout.h

-    explicit DataLayout(int shape_);
+    //! Constructor for the DataLayout
+    /*!
+     * \param shape_ Shape of the index array.


Suggested change

* \param shape_ Shape of the index array.

* \param shape Shape of the index array.

mphoward · 2025-03-06T17:25:03Z

include/flyft/data_layout.h

+    //! Shape of the layout in each dimension
+    std::vector<int> shape() const;
+
+    //! Shape of the layout in each dimension


Suggested change

//! Shape of the layout in each dimension

//! Total number of elements in the layout

mphoward · 2025-03-06T17:25:32Z

include/flyft/data_view.h

+/*!
+ * DataView provides ability to view a certain section of the multidimensional array
+ */
+


Suggested change

mphoward · 2025-03-06T17:27:34Z

include/flyft/data_view.h

+    //! Overloading the constructor
+    /*!
+     * \param data_ Pointer to the data.
+     * \param layout_ Layout of the data.
+     * \param start_ Start of the array.
+     * \param end_ End of the array.
+     */
+
    DataView() : DataView(nullptr, DataLayout()) {}

    DataView(pointer data, const DataLayout& layout) : DataView(data, layout, 0, layout.shape()) {}


You need to document each constructor with the actual arguments of it, as they are named in the signature of the function. Documentation is needed for the third constructor still along with a short description.

Suggested change

//! Overloading the constructor

/*!

* \param data_ Pointer to the data.

* \param layout_ Layout of the data.

* \param start_ Start of the array.

* \param end_ End of the array.

*/

DataView() : DataView(nullptr, DataLayout()) {}

DataView(pointer data, const DataLayout& layout) : DataView(data, layout, 0, layout.shape()) {}

//! Empty constructor

/*!

* The default is a null pointer to a zero-size layout.

*/

DataView() : DataView(nullptr, DataLayout()) {}

//! Constructor

/*!

* \param data Pointer to the data.

* \param layout Layout of the data.

*/

DataView(pointer data, const DataLayout& layout) : DataView(data, layout, 0, layout.shape()) {}

mphoward · 2025-03-06T17:28:03Z

include/flyft/data_view.h

        }

-    reference operator()(int idx) const
+    //! Map multi-dimensional array to one dimension


This is not what this function does... it returns a reference to the referenced element of the data.

mphoward · 2025-03-06T17:29:31Z

include/flyft/data_view.h

+    //! Shape of the one-dimensional array
    int shape() const
        {
        return end_ - start_;
        }

+    //! Number of elements in the one-dimensional array
    int size() const
        {
        return shape();
        }


These are not correct for a multidimensional layout... the shape should be multidimensional. Additionally, I am noticing that start and end need to be generalized to be multi-indexes.

size should be the total number of elements in the view, though.

mphoward · 2025-03-06T17:30:04Z

include/flyft/data_view.h

+    //! Test if pointer to the data is not a null pointer
+    /*!
+     * \return True if pointer is not a null pointer.
+     */
    explicit operator bool() const
        {
        return (data_ != nullptr);
        }


This comment is describing the implementation, not the actual method. A DataView is not null if it is a view of valid data.

…i-dimensional array

mphoward

This looks good to go! I had one small comment: let me know if you want to make that change, and otherwise I'll merge so we can move onto the next steps.

mphoward · 2025-03-18T02:25:47Z

src/data_layout.cc

+    int N = shape_.size();
+    int size = 1;
+    for (int i = 0; i < N; ++i)
+        {
+        size *= shape_[i];
+        }
+    return size;


Could this be done with std::accumulate like you did in data_view.h? I'm good with this solution if you prefer, though!

I will change this to the same implementation as the data_view.h one so that it is consistent. Thank you for noticing it!

mphoward

Thanks! I just created a new branch refactor-aniso that we can use to stage these changes as we work on them. Please base your next branches and PRs off refactor-aniso, thanks!

Add vectors to Data_layout class

71fe635

mayukh33 changed the title ~~Enable two-dimenisonal array in the existing data structure~~ Enable two-dimensional array in the existing data structure Feb 20, 2025

pre-commit-ci bot and others added 2 commits February 20, 2025 21:05

[pre-commit.ci] auto fixes from pre-commit.com hooks

186b3e4

for more information, see https://pre-commit.ci

Add strided indexing for n-dimensional to 1D array mapping

2037bbd

mayukh33 requested a review from mphoward February 21, 2025 16:46

mphoward requested changes Feb 24, 2025

View reviewed changes

Add size calculation and updates for doxygen documentation

3023945

mayukh33 requested a review from mphoward February 25, 2025 16:24

mayukh33 added 2 commits February 26, 2025 10:53

Fix doxygen comment

42a8dfb

Minor fix to data layout comment

423c057

mphoward reviewed Feb 28, 2025

View reviewed changes

Update doxygen documentation

daacc1d

mphoward requested changes Mar 6, 2025

View reviewed changes

mayukh33 added 5 commits March 17, 2025 16:09

Minor fix to Datalayout class

2438d44

Apply changes to make start, end, shape and size compatible with mult…

0027705

…i-dimensional array

Minor fix to data_layout documentation

62e32d4

Fix operator in DataView class

1963b7b

Minor fix

8ea688c

mayukh33 requested a review from mphoward March 17, 2025 21:50

mphoward approved these changes Mar 18, 2025

View reviewed changes

Different implementation of size method in data_layout

24d54aa

mayukh33 requested a review from mphoward March 18, 2025 22:55

mphoward changed the base branch from main to refactor-aniso March 19, 2025 15:57

mphoward approved these changes Mar 19, 2025

View reviewed changes

mphoward merged commit c2dea16 into refactor-aniso Mar 19, 2025
1 check passed

mphoward deleted the feature/ndarray branch March 19, 2025 15:58

		//! Strided indexing scheme for converting
		//! N dimensional array into one dimensional array

		// Getter for the class
		int operator()(const std::vector<int>& idx) const;

-    // Getter for the class
-    int operator()(const std::vector<int>& idx) const;
+    //! Map a multidimensional index to a one-dimensional index
+    /*!
+     * \param idx Multidimensional index.
+     * \return One-dimensional index.
+     *
+     * Row-major ordering is used.
+     */
+    int operator()(const std::vector<int>& idx) const;

	// Getter for shape

	//! Total number of elements in the layout

		bool operator==(const DataLayout& other) const;
		bool operator!=(const DataLayout& other) const;

	std::vector<int> shape_;
	std::vector<int> shape_; //!< Multidimensional shape of layout

	* DataLayout maps an N-dimensional index to one-dimensional
	* DataLayout maps an N-dimensional index to a one-dimensional

	* \param shape_ Shape of the index array.
	* \param shape Shape of the index array.

	//! Shape of the layout in each dimension
	//! Total number of elements in the layout

Conversation

mayukh33 commented Feb 20, 2025

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphoward left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants