Feature/sidre data collection #64

MishaZakharchanka · 2021-04-20T18:43:37Z

Adding functions to store DataTypes into sidre and tests for the those functions.

…e or tested. project compiles

…-collection

methods -- simplest way of dealing with templated methods like this.

…ikeowen/spheral into feature/sidre-data-collection

…-collection

CXXTest Build Fix

mdavis36 · 2021-05-19T02:45:25Z

So if I want to exercise the tests of the new stuff, what do I do? Does "make test" cover it?

From the build dir make test should work

mdavis36 · 2021-05-19T16:20:14Z

src/CXXTests/testSidreStorage.cc

+    }
+};
+
+using MyTypes = ::testing::Types<char, int, size_t, uint32_t, uint64_t, float, double>;


Can we rename this type alias to something more descriptive

mdavis36 · 2021-05-19T16:22:54Z

src/CXXTests/testSidreStorage.cc

+template <typename T>
+class SidreDataCollectionTest : public ::testing::Test
+{
+  public:
+    Spheral::SidreDataCollection myData;
+    int n = 100;
+    T* rawSidreData;
+
+    Spheral::Field<Spheral::Dim<1>, T> makeField()
+    {
+      Spheral::NodeList<Spheral::Dim<1>> makeNodeList("test bed", n, 0);
+      Spheral::Field<Spheral::Dim<1>, T> testField("test field", makeNodeList);
+      for (int i = 0; i < n; i++)
+        testField[i] = i;
+      return testField;
+    }
+
+    void allocRawSidreData(const Spheral::Field<Spheral::Dim<1>, T>& testField)
+    {
+      rawSidreData = myData.alloc_view("SidreTest", testField)->getData();
+    }
+};


Let's move all helper classes, functions, includes and type aliases into a separate header file keeping only the gtest "TEST" type calls in this file.

mdavis36 · 2021-05-19T16:25:59Z

src/Field/Field.hh

@@ -235,6 +236,10 @@ public:
  std::vector<DataType> ghostValues() const;
  std::vector<DataType> allValues() const;

+  // Functions to help with storing the field in a Sidre datastore.
+  axom::sidre::DataTypeId getAxomType() const;


Can we rename this to getAxomTypeID, I thought you were trying to return an actual type here at first instead of the axom ID.

src/Utilities/SidreDataCollectionInline.hh

mdavis36 · 2021-05-19T16:34:41Z

src/CXXTests/CMakeLists.txt

 foreach(test ${gtest_spheral_tests})
    get_filename_component( test_name ${test} NAME_WE )
-    blt_add_executable( NAME ${test_name}_test
+        blt_add_executable( NAME ${test_name}_test


jmikeowen · 2021-05-26T23:25:19Z

In src/Utilities/SidreDataCollection, wouldn't it be preferable to use a std::shared_ptraxom::sidre::DataStore rather than a bare C pointer? This way you can construct using std::make_shared, and not worry about deallocating in the destructor. In general I try to avoid using bare pointers with new/delete just to avoid memory confusion down the line.

…-collection

Sidre data collection test re-org

…-collection

mdavis36 · 2021-08-05T19:19:01Z

src/Utilities/SidreDataCollectionInline.hh

+class SidreDataCollection
+{
+public:
+    SidreDataCollection() {};
+    ~SidreDataCollection() {};
+
+    template <typename Dimension, typename DataType,
+              typename std::enable_if<std::is_arithmetic<DataType>::value,
+                                      DataType>::type* = nullptr>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, DataType> &field);
+
+    template <typename Dimension, typename DataType,
+              typename std::enable_if<is_rank_n_tensor<DataType>::value && !std::is_arithmetic<DataType>::value,
+                                      DataType>::type* = nullptr>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, DataType> &field);
+
+    template <typename Dimension, typename DataType,
+              typename std::enable_if<!is_rank_n_tensor<DataType>::value  && !std::is_arithmetic<DataType>::value,
+                                      DataType>::type* = nullptr>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, DataType> &field);
+
+
+    template<typename Dimension>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, std::string> &field);
+    template<typename Dimension, typename DataType>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, std::vector<DataType>> &field);
+    template<typename Dimension, typename DataType>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, std::tuple<DataType, DataType, DataType>> &field);
+    template<typename Dimension, typename DataType>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, std::tuple<DataType, DataType, DataType, DataType>> &field);
+    template<typename Dimension, typename DataType>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, std::tuple<DataType, DataType, DataType, DataType, DataType>> &field);
+    template<typename Dimension>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, Dim<2>::Vector> &field);
+
+
+private:
+    std::shared_ptr<axom::sidre::DataStore> m_datastore_ptr = std::make_shared<axom::sidre::DataStore>();
+};
+
+} // namespace Spheral


Why is this class declaration duplicated here?

mdavis36 · 2021-08-05T19:19:33Z

src/Utilities/SidreDataCollectionInline.hh

+axom::sidre::Group *SidreDataCollection::sidreStoreField(const std::string &view_name, 
+                                                   const Spheral::Field<Dimension, std::string> &field)


Formatting across this file

mdavis36 · 2021-08-05T19:20:07Z

src/Utilities/SidreDataCollection.hh

+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, DataType> &field);
+
+    template <typename Dimension, typename DataType,
+              typename std::enable_if<is_rank_n_tensor<DataType>::value && !std::is_arithmetic<DataType>::value,
+                                      DataType>::type* = nullptr>
+    axom::sidre::Group *sidreStoreField(const std::string &view_name,
+                                  const Spheral::Field<Dimension, DataType> &field);


MishaZakharchanka · 2021-08-13T16:27:04Z

src/Utilities/SidreDataCollectionInline.hh

+axom::sidre::Group *SidreDataCollection::sidreStoreField(const std::string &view_name,
+                                                         const Spheral::Field<Dimension, std::tuple<DataType, DataType, DataType, DataType, DataType>> &field)


The parameters are lined up, but the second one is very long. Do I need break it up onto more lines?

It's not the prettiest, if you can break it up into a format that maintains the readability go for it, but I don't think it's too much of an issue, this only really effects the tuple cases.

mdavis36 · 2021-08-13T21:06:10Z

@jmikeowen I think we might have the "make test" ats test added in a separate PR as that is going to require us to do some build system trickery to encode the build directory path into the python test file and I'd like to talk about some ideas.

@MishaZakharchanka Please fix the formatting in src/CXXTests/CMakeLists.txt. After that I'll be happy to approve this PR.
LGTM 👍

jmikeowen · 2021-08-13T21:38:41Z

Sure, we can talk about how to add the CXX tests. I'm gonna be traveling until next week, but we can do this sometime then.

…

On Fri, Aug 13, 2021 at 3:06 PM Michael Davis ***@***.***> wrote: @jmikeowen <https://github.com/jmikeowen> I think we might have the "make test" ats test added in a separate PR as that is going to require us to do some build system trickery to encode the build directory path into the python test file and I'd like to talk about some ideas. @MishaZakharchanka <https://github.com/MishaZakharchanka> Please fix the formatting in src/CXXTests/CMakeLists.txt. After that I'll be happy to approve this PR. LGTM 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#64 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCN6BXWD2HPLF4NIS3QGX3T4WCM3ANCNFSM43IZBB5A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

MishaZakharchanka · 2021-09-03T22:11:38Z

As of now this branch is up to date with develop, so we should be good to merge.

mdavis36

LGTM

jmikeowen

Looks good to go.

Mikhail Zakharchanka and others added 30 commits August 28, 2020 17:41

sidre integration start. Basic view creation written, not used in cod…

c0ac876

…e or tested. project compiles

Merge remote-tracking branch 'origin/develop' into feature/sidre-data…

de4aea2

…-collection

import works, sidre test doesnt: function used not found

bdac89c

working towards storing int Fields sidre test

bb3529d

Merge remote-tracking branch 'origin/develop' into feature/sidre-data…

ecd155d

…-collection

Moving the templated methods of SidreDataCollection to inlined

a4a8f54

methods -- simplest way of dealing with templated methods like this.

Merge branch 'feature/sidre-data-collection' of https://github.com/jm…

ccc4f3f

…ikeowen/spheral into feature/sidre-data-collection

working out axom functions overloaded with const

3a5b877

commented out const overloaded func, working on wrapping Group::print().

d627d2a

Int test almost working, everything but Data is reading correctly

59a3d47

Merge remote-tracking branch 'origin/develop' into feature/sidre-data…

e889053

…-collection

Working Sidre test for ints

a8061ab

Trying to build to include gtest, issues with linking.

e502d05

Merge branch 'feature/sidre-data-collection' into python-env-sidre

11605f0

Merge branch 'feature/sidre-data-collection' into python-env-sidre

fbea6d2

Merge branch 'feature/python-env' into python-env-sidre

63546f8

Merge branch 'feature/python-env' into feature/sidre-data-collection

2d05a5c

Merge branch 'feature/sidre-data-collection' into python-env-sidre

671810c

Merge branch 'feature/python-env' into python-env-sidre

4dca95f

Fixing CXXTests CMake merge conflict.

5de3d54

Merge pull request #37 from LLNL/python-env-sidre

aa6ac84

CXXTest Build Fix

Scalar value tests working except char and bool

99f61ed

Vector storage in Sidre working

3a5a168

Field of vectors stored in Sidre works

47e702f

vector working merged with develop

5afe3f0

Missed line comment.

4c590af

Expose SPHERAL_CXX_LIBS porperty to CXXTests.

9dd654c

tuples saved into sidre

a78ef4c

resolved merge conflict.

437b44f

removing deleted header file

f689130

mdavis36 requested changes May 19, 2021

View reviewed changes

MishaZakharchanka and others added 13 commits June 2, 2021 10:38

Problem with NRankTensor not working like other dim types

5712623

Dim<1>ThirdRankTensor storing in sidre working

85c3ec4

merging develope fixed conflict

5aa42c2

Dim2 vector not saving correctly, buffer showing 3th dim

41843c0

Merge remote-tracking branch 'origin/develop' into feature/sidre-data…

a4c437d

…-collection

Dim<2> Vector working with extra function

6f13c4a

Templating out Dimensional value for is_rank_n_tensor type trait.

f2e5ee9

Drawing out re-org prototype for tests.

6896451

Adding AxomType to DataTypeTraits, may not need this later...

8fa1953

Prototype sidre alloc with vector / scalar types.

7b8d4d0

Merge pull request #76 from LLNL/feature/mdavis36/sidre-data-collection

9b0d637

Sidre data collection test re-org

tests changed to be macros and fields now saved into Groups

a741238

Merge remote-tracking branch 'origin/develop' into feature/sidre-data…

888037f

…-collection

mdavis36 requested changes Aug 5, 2021

View reviewed changes

MishaZakharchanka added 2 commits August 9, 2021 10:09

formatting changes

5a77ab5

Merge branch 'develop' into feature/sidre-data-collection

86e8197

MishaZakharchanka commented Aug 13, 2021

View reviewed changes

MishaZakharchanka added 2 commits August 13, 2021 15:50

fixing CXXTests CMake format

0cd2951

Merge branch 'develop' into feature/sidre-data-collection

94d807a

MishaZakharchanka requested a review from mdavis36 August 17, 2021 22:10

mdavis36 approved these changes Sep 8, 2021

View reviewed changes

jmikeowen approved these changes Sep 8, 2021

View reviewed changes

MishaZakharchanka merged commit c53e13d into develop Sep 8, 2021

MishaZakharchanka deleted the feature/sidre-data-collection branch September 14, 2021 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/sidre data collection #64

Feature/sidre data collection #64

MishaZakharchanka commented Apr 20, 2021

mdavis36 commented May 19, 2021

mdavis36 May 19, 2021 •

edited

Loading

mdavis36 May 19, 2021

mdavis36 May 19, 2021

mdavis36 May 19, 2021

mdavis36 Aug 13, 2021

jmikeowen commented May 26, 2021

mdavis36 Aug 5, 2021

mdavis36 Aug 5, 2021

mdavis36 Aug 5, 2021

MishaZakharchanka Aug 13, 2021

mdavis36 Aug 13, 2021

mdavis36 commented Aug 13, 2021

jmikeowen commented Aug 13, 2021 via email

MishaZakharchanka commented Sep 3, 2021

mdavis36 left a comment

jmikeowen left a comment

		axom::sidre::Group *SidreDataCollection::sidreStoreField(const std::string &view_name,
		const Spheral::Field<Dimension, std::string> &field)

		axom::sidre::Group *SidreDataCollection::sidreStoreField(const std::string &view_name,
		const Spheral::Field<Dimension, std::tuple<DataType, DataType, DataType, DataType, DataType>> &field)

Feature/sidre data collection #64

Feature/sidre data collection #64

Conversation

MishaZakharchanka commented Apr 20, 2021

mdavis36 commented May 19, 2021

mdavis36 May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmikeowen commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdavis36 commented Aug 13, 2021

jmikeowen commented Aug 13, 2021 via email

MishaZakharchanka commented Sep 3, 2021

mdavis36 left a comment

Choose a reason for hiding this comment

jmikeowen left a comment

Choose a reason for hiding this comment

mdavis36 May 19, 2021 •

edited

Loading