Fix `MFEMSidreDataCollection::LoadExternalData` and add test cases for various `LoadExternalData` functions #1473

white238 · 2024-11-14T01:30:07Z

Fixes MFEMSidreDataCollection::LoadExternalData to work on the root of the DataStore when in parallel because it is currently required, see Allow IOManager to load external data at specific locations in Sidre hierarchy. #1479
Add tests/examples on how to work with external data in the DataStore, IOManager and MFEMSidreDataCollection classes

white238 · 2024-11-28T00:03:35Z

/style

rhornung67 · 2025-01-09T23:46:17Z

src/axom/sidre/core/MFEMSidreDataCollection.cpp

  #if defined(AXOM_USE_MPI) && defined(MFEM_USE_MPI)
  if(m_comm != MPI_COMM_NULL)
  {
+    // The conduit abstraction appears to automatically handle the ".root"


Would it be cleaner to move the logic for the path suffix into the reader.loadExternalData() method?

I am willing to put this in this PR or make an issue of it. I was just following the previous pattern and advice of @nselliott (i think)

rhornung67 · 2025-01-10T00:17:59Z

@white238 I think by linking issue #1479 in the description of this PR above, the issue will be closed when this PR is merged. We probably don't want that since it describes follow-on work for the solution we really want.

white238 · 2025-01-10T01:56:04Z

@white238 I think by linking issue #1479 in the description of this PR above, the issue will be closed when this PR is merged. We probably don't want that since it describes follow-on work for the solution we really want.

It only closes it if you do a keyword before the issue. See here:

https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

kennyweiss

Thanks @white238

RELEASE-NOTES.md

kennyweiss · 2025-01-10T05:15:36Z

src/axom/sidre/core/MFEMSidreDataCollection.cpp

+      SLIC_ERROR(
+        "Loading external data with a group name is not supported in "
+        "parallel.");


Will this be resolved by #1479 ?

Yes, that is the intention.

I put this in because I kept forgetting and getting no loading and feedback. So I put in an error to warn the user until @nselliott adds the new capability.

src/axom/sidre/tests/sidre_read_write_userdefined_data.cpp

kennyweiss · 2025-01-10T05:29:23Z

src/axom/sidre/tests/sidre_read_write_userdefined_data.cpp

+{
+  test_MFEMSidreDataCollection_user_defined_data<double, 1>();
+}
+/*


Please add a comment about why these are commented out, and add or update an issue to fix these when SPIO can load into the specified group in the MFEMSidreDataCollection

I completely forgot I wrote these then wrote a much simpler single test in the MFEMSidreDataCollection test file..... good catch! They even passed just uncommenting them. Past Me was great!

rhornung67 · 2025-01-10T16:08:18Z

@white238 I think by linking issue #1479 in the description of this PR above, the issue will be closed when this PR is merged. We probably don't want that since it describes follow-on work for the solution we really want.

It only closes it if you do a keyword before the issue. See here:

https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

Ah. Got it.

nselliott · 2025-01-10T17:05:56Z

src/axom/sidre/tests/spio/spio_parallel.hpp

+//------------------------------------------------------------------------------
+TEST(spio_parallel, external_piecemeal_writeread)
+{
+  if(PROTOCOL != "sidre_hdf5")


I have an equivalent test in my work for #1479 , so that will probably replace this in the future, possibly incorporating some pieces of what you have here.

…ead of a file path

…nctions

…error if non-root is requested

Co-authored-by: Kenneth Weiss <[email protected]>

white238 force-pushed the task/white238/quadrature_data_sidre branch from e284561 to 470de37 Compare November 27, 2024 22:34

white238 force-pushed the task/white238/quadrature_data_sidre branch from 9c2795c to 1fbf8dd Compare January 9, 2025 21:29

white238 changed the title ~~WIP: Add test case for array of user-defined struct containing POD data~~ Fix MFEMSidreDataCollection::LoadExternalData and add test cases for various LoadExternalData functions Jan 9, 2025

white238 requested review from kennyweiss, nselliott and rhornung67 January 9, 2025 21:56

white238 added bug Something isn't working Sidre Issues related to Axom's 'sidre' component Spio Issues related to paralle I/O for Axom's Sidre component User Request Issues related to user requests labels Jan 9, 2025

rhornung67 reviewed Jan 9, 2025

View reviewed changes

kennyweiss approved these changes Jan 10, 2025

View reviewed changes

rhornung67 approved these changes Jan 10, 2025

View reviewed changes

nselliott reviewed Jan 10, 2025

View reviewed changes

nselliott approved these changes Jan 10, 2025

View reviewed changes

white238 added 11 commits January 10, 2025 17:37

add tests to build up to eventual use case

18ae6a3

remove unused using's

f6740b8

style

59affef

use the actual type when getting the size, thanks noah!

6cc09d6

add dim, swap for serac's tensor, unify filling and checking whole array

e7fcc2b

fix tests, add tests covering differing size tensor

87e99d8

add comment explaining test

d4a23ce

style

e7f4f75

revert submodule

8870d33

change MFEMSidreDataCollection::LoadExternalData to take a group inst…

60bf2e4

…ead of a file path

add optional group name and have class act on file name like other fu…

1ad4695

…nctions

white238 and others added 16 commits January 10, 2025 17:37

add check for loaded size, add broken test for MFEMSidreDataCollection

a9c21f1

add test for loading external data piecemeal

1aaa3f7

fix test to show failure

a90e98c

remove unused error

b02cd7d

general cleanup

4256802

fix loadexternaldata to work since SPIO currently requires the root, …

2ce97b6

…error if non-root is requested

style

97b9443

Style

ea4da5a

add test for load external data on mfem sidre datacollection

4f04b54

fix double free

ce802f7

style

b986f08

fix release notes

c561c26

fix spio test

b0d6aab

clarify comment

b93b63e

Fix words

44a99b2

Co-authored-by: Kenneth Weiss <[email protected]>

reenable tests i forgot i wrote

3066766

white238 force-pushed the task/white238/quadrature_data_sidre branch from b42ca90 to 3066766 Compare January 11, 2025 01:54

white238 merged commit b811391 into develop Jan 13, 2025
13 checks passed

white238 deleted the task/white238/quadrature_data_sidre branch January 13, 2025 20:46

white238 mentioned this pull request Jan 13, 2025

Review handling/adding of .root in IOManager functions #1489

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `MFEMSidreDataCollection::LoadExternalData` and add test cases for various `LoadExternalData` functions #1473

Fix `MFEMSidreDataCollection::LoadExternalData` and add test cases for various `LoadExternalData` functions #1473

white238 commented Nov 14, 2024 •

edited

Loading

white238 commented Nov 28, 2024

rhornung67 Jan 9, 2025 •

edited

Loading

white238 Jan 11, 2025

rhornung67 commented Jan 10, 2025

white238 commented Jan 10, 2025

kennyweiss left a comment

kennyweiss Jan 10, 2025

nselliott Jan 10, 2025

white238 Jan 11, 2025

kennyweiss Jan 10, 2025

white238 Jan 11, 2025

rhornung67 commented Jan 10, 2025

nselliott Jan 10, 2025 •

edited

Loading

Fix MFEMSidreDataCollection::LoadExternalData and add test cases for various LoadExternalData functions #1473

Fix MFEMSidreDataCollection::LoadExternalData and add test cases for various LoadExternalData functions #1473

Conversation

white238 commented Nov 14, 2024 • edited Loading

white238 commented Nov 28, 2024

rhornung67 Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

white238 Jan 11, 2025

Choose a reason for hiding this comment

rhornung67 commented Jan 10, 2025

white238 commented Jan 10, 2025

kennyweiss left a comment

Choose a reason for hiding this comment

kennyweiss Jan 10, 2025

Choose a reason for hiding this comment

nselliott Jan 10, 2025

Choose a reason for hiding this comment

white238 Jan 11, 2025

Choose a reason for hiding this comment

kennyweiss Jan 10, 2025

Choose a reason for hiding this comment

white238 Jan 11, 2025

Choose a reason for hiding this comment

rhornung67 commented Jan 10, 2025

nselliott Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Fix `MFEMSidreDataCollection::LoadExternalData` and add test cases for various `LoadExternalData` functions #1473

Fix `MFEMSidreDataCollection::LoadExternalData` and add test cases for various `LoadExternalData` functions #1473

white238 commented Nov 14, 2024 •

edited

Loading

rhornung67 Jan 9, 2025 •

edited

Loading

nselliott Jan 10, 2025 •

edited

Loading