Feature/sina/hdf5 output support #1480

doutriaux1 · 2024-12-20T00:09:23Z

Summary

This PR is a feature
It does the following (modify list as needed):
- Adds the capacity to output sina files as hdf5 and opens the door for more formats in the future

cyrush · 2024-12-20T00:13:08Z

src/axom/sina/core/Document.cpp

-      message << "The '" << RELATIONSHIPS_KEY
-              << "' element of a document must be an array";
-      throw std::invalid_argument(message.str());
+        conduit::Node relationship_nodes = asNode[RELATIONSHIPS_KEY];


this should be a ref to avoid a copy

Removed as many copies as I could, this one underwent additional changes

cyrush · 2024-12-20T00:19:56Z

src/axom/sina/core/Document.cpp

+        conduit::Node modifiedRelationshipsNode;
+
+        removeSlashes(relationshipNode, modifiedRelationshipsNode);
+        relationshipsNode.append() = modifiedRelationshipsNode;


you could pass ref to append result into removeSlashes to avoid one extra copy.

Copy removed

are the changes pushed? I see:

removeSlashes(relationshipNode, modifiedRelationshipsNode); relationshipsNode.append() = modifiedRelationshipsNode;

however, I expect something like:

removeSlashes(relationshipNode, relationshipsNode.append());

@cyrush @gwaegner cannot push. I just pushed the changes.

cyrush · 2024-12-20T01:22:34Z

src/spack

was this sub module add intended?

Submodule removed

Looks like the PR is trying to make unintended changes to submodules (blt, data, radiuss-spack-configs)

Submodule changes came from develop merges, double checked for intentionality

bgunnar5

Left some suggestions for you. Also wondering if the fortran changes are tested?

src/axom/sina/core/Document.cpp

src/axom/sina/core/Document.hpp

src/axom/sina/docs/sphinx/tutorial.rst

src/axom/sina/examples/sina_tutorial.cpp

…tion

doutriaux1 · 2025-01-26T16:40:50Z

@gwaegner please take a look at the failures

bmhan12

The updated ReadTheDocs documentation for this PR can be previewed here: https://axom.readthedocs.io/en/feature-sina-hdf5_output_support/axom/sina/docs/sphinx/index.html

bmhan12 · 2025-01-28T16:48:07Z

src/axom/sina/core/Document.hpp

+  /**
+   *  \brief Dump this document as an HDF5 File
+   * 
+   *  \return None, conduit automatically dumps the hdf5 file without a return
+   */


Maybe instead of \return..., this comment can be something like:
\param [out] filename hdf5 file to dump the document

Documentation updated

bmhan12 · 2025-01-28T17:01:46Z

src/axom/sina/core/Document.hpp

@@ -203,9 +226,10 @@ class Document
 *
 * \param document the Document to save
 * \param fileName the location to which to save the file
+ * \param protocol the file type requested to save as, default = JSON


Maybe want to note here that HDF5 file type is also supported, or update Document class description.

Documentation updated

bmhan12 · 2025-01-28T17:03:45Z

src/axom/sina/core/Document.hpp

 /**
 * \brief An object representing the top-level object of a Sina JSON file
 *


Should the class header description for Document be updated, generalized to include hdf5?

Documentation updated

doutriaux1 · 2025-02-10T20:17:14Z

@kennyweiss I think @gwaegner adressed everything and the tests are now passing. Can you please take on last look to make sure we covered everything. Thanks!

Must include `axom/config.hpp` before checking defines like `AXOM_USE_HDF5`

kennyweiss

Thanks for updating the branch @gwaegner

I built it locally and fixed the submodules and includes for axom/config.hpp, which needs to be brought in before checking for AXOM_USE_HDF5. I also added a few guards around uses of axom::Protocol::HDF5.

I think this branch still needs a bit more work to handle configurations without hdf5. Specifically, one decision is about whether sina::Protocol::HDF5 should always be defined or if it should only be defined when AXOM_USE_HDF5. In the former case, users can use the enum value without guarding against HDF5, but need some runtime checks or try/catch loops to handle cases where it's not available. Another issue relates to how the Fortran wrapper and examples handle this case.

I tested the branch by building a library set without hdf5 --

> ./scripts/uberenv/uberenv.py --spec "%[email protected]+devtools~hdf5+c2c~scr+profiling  ^conduit~hdf5"

and then configuring axom with the host-config file that it generated.

In doing so, I discovered some test failures in sidre and quest tests. I'll fix these in a separate branch.

kennyweiss · 2025-02-19T00:29:53Z

src/axom/sina/core/Document.cpp

@@ -15,13 +15,24 @@

 #include "axom/sina/core/Document.hpp"

+#include "axom/config.hpp"


Note: We need to #include "axom/config.hpp" to bring in compiler defines such as AXOM_USE_HDF5. If we don't, they will always appear to be off.

I updated the includes in this branch to bring in axom/config.hpp

src/axom/sina/core/Document.hpp

kennyweiss · 2025-02-19T00:34:18Z

src/axom/sina/docs/sphinx/documents.rst

+    #include "axom/sina.hpp"
+
+    int main (void) {
+        axom::sina::Document myDocument = axom::sina::loadDocument("MySinaData.hdf5", axom::sina::Protocol::HDF5);


Note: W/ the current implementation, this snippet will raise a compilation error when the code is built without HDF5, so effectively, HDF5 is required to build the code!

HDF5 is always defined so there shouldn't be a compilation error

HDF5 is always defined so there shouldn't be a compilation error

I'm not sure I understand this.
Are you saying it's always defined in our configurations that build the docs?

I'm suggesting either adding an ifdef guard to this snippet, or mentioning in the surrounding text that this snippet assumes axom is built w/ hdf5 support.

src/axom/sina/examples/sina_basic.cpp

src/axom/sina/examples/sina_fortran.f

src/axom/sina/core/Document.hpp

src/axom/sina/core/Document.cpp

kennyweiss · 2025-02-19T00:47:11Z

src/axom/sina/core/Document.cpp

+  else
+  {
+    throw std::invalid_argument(
+      "Invalid format choice. Please enter 'json' or 'hdf5'.");
+  }


Can the code ever get to this else?

Similar to the above default, the code will hit this else if a user attempts Protocol::HDF5 with no HDF5 support since the else if(Protocol::HDF5) case will not be loaded

src/axom/sina/interface/sina_fortran_interface.cpp

kennyweiss · 2025-02-19T04:12:12Z

Note: I created a PR to fix some tests for Axom configs without hdf5 (independent of this PR): #1504

doutriaux1 · 2025-02-19T05:23:23Z

I've seen that. Thanks @kennyweiss . @gwaegner please take a look at Kenny's PR as well to see how he took care of things there.

gwaegner · 2025-03-05T17:54:48Z

All requested changes should be accounted for. Is someone able to give this another look over?

kennyweiss

Thanks @gwaegner for incorporating all the suggestions.

I added a few more relatively small comments.
My only remaining concern is about how sina is installed, which should be easy to remedy. @white238 -- would you mind looking at the CMake files?

Please also update the RELEASE_NOTES to add a note about the new HDF5 output support.

Also, when it's ready, this PR will have to be squashed merged due to the submodule changes in its history.

kennyweiss · 2025-03-04T18:56:09Z

src/axom/sina/CMakeLists.txt

  blt_list_append( TO sina_sources
-                   ELEMENTS interface/sina_fortran_interface.cpp interface/sina_fortran_interface.f)
+  ELEMENTS interface/sina_fortran_interface.cpp interface/sina_fortran_interface.f.in)


I don't think we want to include the template file interface/sina_fortran_interface.f.in,

Updated to remove .f.in's inclusion

kennyweiss · 2025-03-04T18:56:52Z

src/axom/sina/CMakeLists.txt

 if (ENABLE_FORTRAN)
  blt_list_append( TO sina_headers ELEMENTS interface/sina_fortran_interface.h)
  blt_list_append( TO sina_sources
-                   ELEMENTS interface/sina_fortran_interface.cpp interface/sina_fortran_interface.f)
+  ELEMENTS interface/sina_fortran_interface.cpp interface/sina_fortran_interface.f.in)
+  configure_file(interface/sina_fortran_interface.f.in interface/sina_fortran_interface.f @ONLY)


We'll likely also need to install the generated file interface/sina_fortran_interface.f

.f file is now explicitly installed.

kennyweiss · 2025-03-04T18:57:39Z

src/axom/sina/CMakeLists.txt

+if(AXOM_USE_HDF5 AND ENABLE_FORTRAN)
+  set(AXOM_USE_HDF5_FORTRAN ".true.")
+else()
+  set(AXOM_USE_HDF5_FORTRAN ".false.")
+endif()


As an aside, exposing the enum values will be a lot cleaner w/ shroud

@kennyweiss yes at some point we should convert to shroud as a separate PR. But it's kind of "if it's ain't broken why fix it?" at the moment 😉

kennyweiss · 2025-03-05T18:37:45Z

src/axom/sina/tests/test_fortran_integration.py

+try:
+    import h5py
+    AXOM_USE_HDF5 = True
+except ImportError:
+    AXOM_USE_HDF5 = False


Is there a better way to do this?
It seems like this will depend more on the user's python environment than on their Axom config.

Configured to now base AXOM_USE_HDF5 on the preprocessor version using a config.py.in file

kennyweiss · 2025-03-05T18:38:16Z

src/axom/sina/tests/test_fortran_integration.py

@@ -116,8 +123,153 @@ def test_validate_contents_of_record(self):
        self.assertEqual(double_arr, record["curve_sets"][curveset]["dependent"][double_2_name]["value"])


+#HDF5 Test
+@unittest.skipUnless(AXOM_USE_HDF5, "Requires h5py for HDF5-dependent tests")


This is a nice solution!

kennyweiss · 2025-03-05T18:48:00Z

src/axom/sina/docs/sphinx/hdf5_vs_json.rst

+
+* HDF5 offers better size and speed efficiency when dealing with larger files/curve sets 
+and only outperforms more dramatically as size increases
+* Hierarchal structure leads to being 2.5x more size efficient and 5x faster at our largest


Suggested change

* Hierarchal structure leads to being 2.5x more size efficient and 5x faster at our largest

* Hierarchical structure leads to being 2.5x more size efficient and 5x faster at our largest

kennyweiss · 2025-03-05T18:48:20Z

src/axom/sina/docs/sphinx/hdf5_vs_json.rst

@@ -0,0 +1,55 @@
+.. ## Copyright (c) 2017-2024, Lawrence Livermore National Security, LLC and


Please run the update copyright script on this branch

kennyweiss · 2025-03-05T18:51:38Z

src/axom/sina/docs/sphinx/documents.rst

+    #include "axom/sina.hpp"
+
+    int main (void) {
+        axom::sina::Document myDocument = axom::sina::loadDocument("MySinaData.hdf5", axom::sina::Protocol::HDF5);


HDF5 is always defined so there shouldn't be a compilation error

I'm not sure I understand this.
Are you saying it's always defined in our configurations that build the docs?

I'm suggesting either adding an ifdef guard to this snippet, or mentioning in the surrounding text that this snippet assumes axom is built w/ hdf5 support.

kennyweiss · 2025-03-05T18:52:55Z

src/axom/sina/core/Document.hpp

+enum class Protocol
+{
+  JSON,
+  HDF5
+};
+
+const std::vector<std::string> supported_types = {"JSON",
+#ifdef AXOM_USE_HDF5
+                                                  "HDF5"
+#endif
+};


kennyweiss · 2025-03-05T18:55:10Z

src/axom/sina/core/Document.cpp

+  if(protocol == Protocol::JSON)
+  {
+    protocolWarn(".json", fileName);
+    auto asJson = document.toJson();
+    std::ofstream fout {tmpFileName};
+    fout.exceptions(std::ostream::failbit | std::ostream::badbit);
+    fout << asJson;
+    fout.close();
+  }
+#ifdef AXOM_USE_HDF5
+  else if(protocol == Protocol::HDF5)
+  {
+    protocolWarn(".hdf5", fileName);
+    document.toHDF5(tmpFileName);
+  }
+#endif
+  else
+  {
+    std::ostringstream message;
+    message << "Invalid format choice. Please choose from one of the supported "
+               "protocols: "
+            << get_supported_file_types();
+    throw std::invalid_argument(message.str());
+  }


Minor (consistency):
Is there a reason to use a switch in loadDocument() but if/else conditions in saveDocument()?

Switch statements do not allow us to instantiate variables within cases which causes problems for the JSON case. If we used a switch statement we would end up running auto asJson = document.toJson() even when saving as HDF5 or getting an unsupported protocol.

doutriaux1 and others added 6 commits December 18, 2024 16:19

created a clean new branch from Gabriel's work to push to github repo

3f09c5e

Comments Implemented

e0cd5bf

More Comments

d2f38ac

Fix?

6c32398

Fix??

31242a6

Final Fix

aa9073d

doutriaux1 requested review from white238, cyrush, kennyweiss, rhornung67, bgunnar5 and HaluskaR December 20, 2024 00:09

cyrush reviewed Dec 20, 2024

View reviewed changes

Axom Changes

b553c44

bgunnar5 reviewed Dec 21, 2024

View reviewed changes

gwaegner and others added 7 commits January 8, 2025 14:55

Brian's Changes, Wait to hear back on createFromNode

c5c0cb4

Merge branch 'develop' into sina_hdf5_implementation

8dd8a84

Create From Node Changes

8e6afc1

Forgot to delete some leftover comments

247591f

Small fix

703dbd1

Merge remote-tracking branch 'llnl/develop' into sina_hdf5_implementa…

e1d7631

…tion

merged github's develop in

0df65b3

Pipeline Fix

e118d24

kennyweiss requested a review from bmhan12 January 27, 2025 22:19

bmhan12 reviewed Jan 28, 2025

View reviewed changes

gwaegner added 2 commits January 28, 2025 09:26

Clang Format

0f5851e

More ClangFormat

1168000

doutriaux1 and others added 4 commits February 4, 2025 11:25

bringing in submodule updates

ccfa513

Build Fix

c05979d

Documentation Update

7c5d7cc

Merge branch 'develop' into feature/sina/hdf5_output_support

d036b67

kennyweiss added 4 commits February 18, 2025 14:26

Fixes submodule pointers

79f3729

Merge branch 'develop' into feature/sina/hdf5_output_support

f92080d

Fixes includes in sina

4310ee1

Must include `axom/config.hpp` before checking defines like `AXOM_USE_HDF5`

Guards usage of sina::Protocol::HDF5

0fd3a35

kennyweiss requested changes Feb 19, 2025

View reviewed changes

kennyweiss mentioned this pull request Feb 19, 2025

Fixes build and tests when not using hdf5 #1504

Merged

gwaegner and others added 14 commits February 21, 2025 17:10

Guarding and Fortran tests

1145c29

Merge branch 'develop' into feature/sina/hdf5_output_support

4043d3e

submodule fix?

3ccfbb3

more submodule fix

9429b60

Fix Fortran Test

45ee75b

CMakeLists Fix

0f016bb

Minor Fix

4a2cf7e

Another minor change

2499896

Maybe this change?

209ee7d

Syntax fixes

35bf45b

Test Update

64e5026

Syntax Fix

a0341da

Test Pipeline for Not HDF5

baf2492

Reset

3e03355

kennyweiss requested changes Mar 5, 2025

View reviewed changes

New Changes

8370a21

		@@ -15,13 +15,24 @@

		#include "axom/sina/core/Document.hpp"

		#include "axom/config.hpp"

	* Hierarchal structure leads to being 2.5x more size efficient and 5x faster at our largest
	* Hierarchical structure leads to being 2.5x more size efficient and 5x faster at our largest

		@@ -0,0 +1,55 @@
		.. ## Copyright (c) 2017-2024, Lawrence Livermore National Security, LLC and

Feature/sina/hdf5 output support #1480

Are you sure you want to change the base?

Feature/sina/hdf5 output support #1480

Conversation

doutriaux1 commented Dec 20, 2024

Summary

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

cyrush Jan 24, 2025 • 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

Choose a reason for hiding this comment

bgunnar5 left a comment

Choose a reason for hiding this comment

doutriaux1 commented Jan 26, 2025

bmhan12 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doutriaux1 commented Feb 10, 2025

kennyweiss left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwaegner Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

kennyweiss commented Feb 19, 2025

doutriaux1 commented Feb 19, 2025

gwaegner commented Mar 5, 2025

kennyweiss left a 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

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

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

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

cyrush Jan 24, 2025 •

edited

Loading

gwaegner Feb 28, 2025 •

edited

Loading