Dynamic topology phase IV #481

tokusanya · 2024-08-15T20:34:06Z

Functionality for reduction operations on groups

…haseIV

tokusanya · 2024-08-15T20:36:52Z

packages/seacas/libraries/ioss/src/exodus/Ioex_DatabaseIO.C

+              this_region->add_state_nl(tsteps[i] * timeScaleFactor);
+            }
+
+            tsteps[i] *= timeScaleFactor;


@gsjaardema It is unclear if to return the original time or the scaled time ...

I think scaled time.

gsjaardema

Looks good. A few questions/comments/suggestions.

packages/seacas/libraries/ioss/src/Ioss_Region.C

gsjaardema · 2024-08-15T22:18:58Z

packages/seacas/libraries/ioss/src/Ioss_Region.C

@@ -3055,4 +3105,28 @@ namespace Ioss {
      topologyObserver->reset_topology_modification();
    }
  }
+
+  std::string Region::get_group_name() const


Is this needed by external clients... Not sure it makes sense to expose the concept of a "group" to external clients since it is an artifact of how we are using exodus. Is it possible to isolate groups at the exodus level. If not, is there some more descriptive name that we can use at the higher levels...

It probably doesn't need to exposed externally .. I'll make it a private/function. We probably need a better descriptor for groups since the concept of grouping does exist at the Region level where you specify file control but not all databases support "groups" even though we could treat the current behaviors as having only "one" possible group

gsjaardema · 2024-08-15T22:20:34Z

packages/seacas/libraries/ioss/src/Ioss_Region.C

+    return get_database()->get_group_name();
+  }
+
+  std::tuple<std::string, int, double> Region::locate_db_state(const double targetTime)


Does this only work for the cases where the exodus file contains groups, or will it return the "group" correspoinding to a set of files -s000X storing topology changed data...

Yeah, this is really only for group files but I might be able to modify it to scan the multi-file case....the return string would have to be the database name, I guess

I guess it depends on how complete and broad we are trying to make the capability. I don't know what the clients will expect -- I could see them wanting to use the same interface for all database types (that support restart/output) whether they use groups or different files, or a different method all together.... If it doesn't look too difficult then might be worthwhile...

gsjaardema · 2024-08-15T22:27:34Z

packages/seacas/libraries/ioss/src/exodus/Ioex_DatabaseIO.C

+              this_region->add_state_nl(tsteps[i] * timeScaleFactor);
+            }
+
+            tsteps[i] *= timeScaleFactor;


I think scaled time.

packages/seacas/libraries/ioss/src/gen_struc/Iogs_DatabaseIO.C

packages/seacas/libraries/ioss/src/generated/Iogn_DatabaseIO.C

packages/seacas/libraries/ioss/src/text_mesh/Iotm_DatabaseIO.C

gsjaardema · 2024-08-15T22:38:39Z

packages/seacas/libraries/ioss/src/unit_tests/UnitTestDynamicTopology.C

@@ -1106,6 +1108,57 @@ TEST(TestDynamicRead, single_file_simple_topology_modification)
  cleanup_single_file(outFile);
 }

+std::tuple<std::string, int, double> read_and_locate_db_state(const OutputParams& params, const double targetTime)


Might be good to add more tests -- one with all negative times, one with negative and positive. Also have times in random order 5, 1, 4 to simulate the cyclic time option...

Yup, will do

I thought the CYCLIC_FILE option only referred to the creation of the filenames based on a modulo reduction. How does it affect the timestep since I thought the timesteps associated with states always had to be monotone increasing. I typically see warning messages if the times are in random order

It is posible to use the cyclic option on timesteps within a single file in which case the timesteps would not be monotonically increasing. There is a warning, but it is allowed. I think CYCLIC_FILE puts each timestep in a single file and then reuses those files, but there is also a way that a single file will be used and timesteps overwritten.

For example, if cycle is set to 3 then there would only be 3 timesteps in the file and they would be 1, 2, 3. Then 1 would be overwritten with 4, 2 with 5, 3 with 6,. Then 4 with 7, ...

This is in Ioss_DatabaseIO.h:

/*! Typically used for restart output, but can be used for all output... * Maximum number of states on the output file. Overwrite the existing * steps in a cyclic manner once exceed this count. Note that this breaks * the convention that times be monotonically increasing on an exodusII file. * Used by derived classes if they support this capability... */ mutable int cycleCount{0};

gsjaardema · 2024-08-19T15:13:08Z

packages/seacas/libraries/exodus/src/ex_inquire.c

@@ -975,6 +975,18 @@ static int ex_inquire_internal(int exoid, int req_info, int64_t *ret_int, float
 #endif
    break;

+  case EX_INQ_FILE_FORMAT:


If this stays, then it should also distinguish between all the different formats. classic, netcdf4-classic, netcdf4, netcd3-large-model, netcdf5.

I'm not sure it is needed though, see comment below...

gsjaardema · 2024-08-19T15:13:28Z

packages/seacas/libraries/ioss/src/exodus/Ioex_BaseDatabaseIO.C

@@ -502,6 +502,24 @@ namespace Ioex {
    ex_set_max_name_length(m_exodusFilePtr, maximumNameLength);
  }

+  bool BaseDatabaseIO::supports_group_nl()


I'm not sure this is the correct way to do this... Basically, prior to creating the database, we want to know if the database type specified by the user supports groups -- exodus, cgns, adios, faodal, ... Not whether a specific database file that we have already created.

So, we just have to know whether exodus was compiled with netcdf-4 capability enabled and then EXODUS output type would return true for supporting groups and we could then created a "grouped database".

This would also then be a compile-time check instead of runtime and we would not need the locking and _nl types.

I think checking after the database is created is too late and doesn't give the client the information that they want. If they want to use groups and the chosen format supports groups, then they can create the database (and we make sure that netcdf-4 format is selected as we currently do).

I get what you are saying here but it appears that we may have different interpretations of what this function is supposed to do. I think you are right that the change-set concept that is handled with groups in exodus is spilling out and needs to be made database-agnostic.

Based on the usage of this function in the unit tests, my goal was to find a way to do reduction operations on the databases depending on whether an input file was written in a format that supports groups (as opposed to knowing if the currently compiled database format can support groups). If the input file format doesn't support groups (or change-sets as we are now referring to them), then we do reductions across a set of files which may be linear or cyclic , otherwise we do the reduction across the change-sets within the file.

I would really like to meet up to have a more in-depth discussion on these concepts and API

gsjaardema · 2024-08-19T15:18:30Z

packages/seacas/libraries/ioss/src/exodus/Ioex_DatabaseIO.C

@@ -648,6 +648,7 @@ namespace Ioex {
    int                 tstepCount     = 0;
    std::vector<double> tsteps(0);

+    // Use reference to make sure that no Region modifications occur based on the input flag


I would say something like the setRegionTimeSteps flag determines whether we are actually populating the region timesteps or just querying the timesteps that are on a specific database without populating the regions timesteps data and setting the number of timesteps on the region...

Maybe not much better than the coment above, but maybe says a little more about why a client would want to do that...

gsjaardema · 2024-08-19T15:32:51Z

packages/seacas/libraries/ioss/src/Ioss_DatabaseIO.h

+     *
+     *  \returns True if successful.
+     */
+    bool supports_group()


I think this is exposing the method that we are using in exodus through netCDF -- groups as a general concept in the IOSS API.

I think that instead the client knows that their output/model is going to have topology changes and want to store that in the best way possible. One way is that they know they are using exodus as the output format and they either want single file output or each change in a separate file. They don't care about how this is done internally in the file.

I don't know what our API-visible interface for this should be, but groups is very overloaded term and could be confused with assemblies. Or, to put another way, groups doesn't suggest to me any association with changing topology.

It may be ok to use group down at the exodus level, but if cgns supports something like delta-sets as their method for topology changes then we have confusion in the API...

I'm not sure what to suggest, but seeing all this group-related api functions at this level just seems wrong...

Maybe something like change-set or ? -- All database type could/might support this. In some it is a set of appropriately-named files; in some it uses the netCDF groups, if some other type each file contains full model topology and there is some metadata file that says what happened from file to file...

At creation, the client specifies that topology changes will happen, so IOSS does the appropriate action (whatever that might be). On read, IOSS queries the database(s) and can report that there are topology changess or multiple "change-sets".

Note that the "change-sets" might all contain the same topology -- is this is a run with multiple restarts, each restart state might be in a separate "change-set" even though the topology from one to another is the same.

I think we have the basics of this working in exodus at this point, but now need to plan for how the client interfaces with the concept and how the concept gets expanded out as a "database-agnostic" concept that encompasses and encapsulates as much similar behavior as possible. It also needs to plan for extension.

I don't know what that API view looks like, but I don't think "groups" is the correct one.

@gdsjaar I think we probably need to meet to discuss these concepts and nail down what the API should eventually look like

…haseIV

gsjaardema

Lots of good stuff here. I didn't make it through everything yet, but tried to get a good overview. I think it looks good overall and is evolving toward where I think we want to go...

gsjaardema · 2024-08-27T13:49:08Z

packages/seacas/libraries/ioss/src/Ioss_DatabaseIO.h

@@ -246,63 +246,75 @@ namespace Ioss {
      flush_database_nl();
    }

-    /** \brief If a database type supports groups and if the database
-     *         contains groups, open the specified group.
+    /** \brief If a database type supports change sets and if the database


If the change set is a concept at the IOSS level and can consist of either internal groups (exodus) or suitably named files (exodus, cgns, ...), then it isn't really a function of the database type supporting change sets...

There is still a need to know the database type so the IOSS can determine which method to use for the change sets I guess...

Yes, I wanted change_set to be an IOSS concept that encapsulates the separation layer between it and the various formats. However, the separation I originally had in mind was the internal implementation of the "multiple-file-per-database" concept (which is done through groups in Exodus. Upon refactoring a lot of the functiionality in Ioss_DynamicTopology.C, I realized that change_set could be a more generalized concept and had actually started trying to write a ChangeSet class that would generalize the "group" file, linear multi-file and cyclic multi-file ideas. If that sounds like a good idea, then it nicely dovetails into what you mentioned above regarding knowing the database type for IOSS to choose what method to use. However, in a case like exodus which supports both group and multi file, you would need finer granularity to tell IOSS which one to use

Yes, that is what I thought we had decided on when change-set was originally proposed:

Maybe something like change-set or ? -- All database type could/might support this. In some it is a set of appropriately-named files; in some it uses the netCDF groups, if some other type each file contains full model topology and there is some metadata file that says what happened from file to file...

gsjaardema · 2024-08-27T13:50:32Z

packages/seacas/libraries/ioss/src/Ioss_DatabaseIO.h

    }

-    Ioss::NameList groups_describe(bool return_full_names = false)
+    /** \brief Checks if a database type supports changes sets


spelling -- changes to change

gsjaardema · 2024-08-27T13:52:06Z

packages/seacas/libraries/ioss/src/Ioss_DatabaseIO.h

+     *
+     *  \returns True if successful.
+     */
+    bool supports_change_set()


This is probably needed, but since the IOSS can make any database type support change sets, then maybe this needs to return an enum of what type of change sets is/are supported -- internal, set of files, ...

As per my comment above, if we are generalizing change_set to mean internal and multi-files, then the meaning of this function changes. I initially wrote it to mean that the underlying format supports internal files but I can try to extend it to the more general idea that you proposed

gsjaardema · 2024-08-27T13:54:54Z

packages/seacas/libraries/ioss/src/Ioss_DatabaseIO.h

+     *         controlling properties.
+     *
+     *  This is different from get_step_times() in that it does not set timestep
+     *  data on the region. If the database supports groups, it will return the


maybe change set instead of group

gsjaardema · 2024-08-27T13:57:14Z

packages/seacas/libraries/ioss/src/Ioss_DatabaseIO.h

-    virtual bool open_group_nl(const std::string & /* group_name */) { return false; }
-    virtual bool create_subgroup_nl(const std::string & /* group_name */) { return false; }
-    virtual Ioss::NameList groups_describe_nl(bool /* return_full_names */)
+    virtual bool supports_change_set_nl() { return false; }


Similar question re what does "supports change sets" actually mean if they can be simulated/controlled at the IOSS level...

gsjaardema · 2024-08-27T14:17:07Z

packages/seacas/libraries/ioss/src/Ioss_DynamicTopology.C

+                                 util.communicator(), properties);
+    int bad_count = 0;
+    std::string error_message;
+    bool is_exodus_file = db != nullptr && db->ok(false, &error_message, &bad_count);


Why does exodus appear here? Can it be type agnostice?

I'll remove references to exodus ... it's how it appeared in Framework IO 😬

gsjaardema · 2024-08-27T14:19:40Z

packages/seacas/libraries/ioss/src/Ioss_DynamicTopology.C

+                                  DatabaseState& loc)
+{
+  auto util = db->util();
+  FileNameGenerator generator = [](const std::string& baseFileName, unsigned step) {


Maybe I'm misreading something here, but isn't this casting the type of a lambda into a std::function (or whatever FileNameGenerator is)?

I believe this is standard where you construct a std::function from a lambda. It's not a cast

gsjaardema · 2024-08-27T14:22:20Z

packages/seacas/libraries/ioss/src/Ioss_DynamicTopology.C

+    if(step == 0)
+      step++;
+
+    static std::string suffix = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";


Seems like we have this in multiple places... Maybe one is for input and one for output...

Are you talking about line 865 or the function?

The line is what caught my attention, but I think the same functionality of generating a cyclic file name is present in many locations; especially now that we have pulled some framework code down into IOSS. Maybe now is time to consolidate into a single function if possible.

Ok, I'll look into consolidation

gsjaardema · 2024-08-27T14:27:25Z

packages/seacas/libraries/ioss/src/Ioss_Region.C

+  {
+    IOSS_FUNC_ENTER(m_);
+    auto db = get_database();
+    if (!db->is_input() && db->usage() != WRITE_RESULTS && db->usage() != WRITE_RESTART) {


I guess this is for heartbeat and history?

I wonder if we can simplify that line for input only ... can an input file have usage of WRITE_RESULTS or WRITE_RESTART ?

It is for non-input (i.e. output) and I think the only output tyeps are WRITE_{results|restart|history|heartbeat}
(packages/seacas/libraries/ioss/src/Ioss_DBUsage.h)

So yeah, this function is meant for RESULTS and RESTART, hence the early return ... not sure if to limit it to output (WRITE_*) though

packages/seacas/libraries/ioss/src/exodus/Ioex_ParallelDatabaseIO.C

…haseIV

gsjaardema

There are a few suggestions for changes. Nothing required that I remember... Looks good although lots of code, so not sure I fully covered all of it...

gsjaardema · 2024-09-04T20:49:33Z

packages/seacas/libraries/ioss/src/Ioss_ChangeSetFactory.h

+
+  static int                        describe(NameList *names);
+  IOSS_NODISCARD static NameList    describe();
+  static void                       clean();


What does this do?

The ChangeSetFactory or those functions? A lot of the design for the factory is based on IOFactory that implements those functions

Yeah, clean in this case does nothing ... I don't think even the Exodus version needs it. I can take it out for now

packages/seacas/libraries/ioss/src/Ioss_ChangeSet.C

packages/seacas/libraries/ioss/src/Ioss_ChangeSet.h

packages/seacas/libraries/ioss/src/Ioss_DynamicTopology.C

packages/seacas/libraries/ioss/src/Ioss_DynamicTopology.h

packages/seacas/libraries/ioss/src/exodus/Ioex_ChangeSet.h

packages/seacas/libraries/ioss/src/Ioss_Region.C

Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.9.1 to 2.10.0. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@5c7944e...446798f) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* EXODUS: Add support for lossy compression via netCDF quantize method * IOSS: Add support for lossy compression specifying NSD for netCDF quantize method * EXODUS: Update version date * EXODUS: Add some error checking on nsd values * EXODUS: Check if Quantization supported; include in config output * EXODUS: Cleaning up new compression support * EXODUS: Fix error handling code * EXODUS: Better error checking on compression * IOSS: Support for zstd compression * IOSS: Handle building with no netCDF-4 capability * IOSS: Change gold files to netcdf-3 so work without hdf5-supported netCDF * CI: See if can figure out intel build failure * CI: Fix hdf5 version warning message * CI: Turn on plugins; support new syntax * EXODUS: Only flush stderr if written to Signed-off-by: Greg Sjaardema <gdsjaar@sandia.gov> * Tweak compression additions * CI: Fix intel build script syntax error * EXODUS: Support new netCDF with changed _FillValue * IOSS: Fix so latest fmt version works * IOSS: Catalyst API 2 (sandialabs#463) * IOSS: Catalyst API 2 Added support to database for using my_processor and processor_count IOSS properties when operating in serial parallel mode and reading Conduit files. * IOSS: Catalyst API 2 Changed Conduit serial parallel test to use files from the one MPI rank can.ex2 test. --------- Co-authored-by: Greg Sjaardema <gsjaardema@gmail.com> * Fix export symbols (sandialabs#465) * IOSS: Catalyst API 2 (sandialabs#466) Create the cell_ids and cell_node_ids fields for a StructuredBlock when asked for by a reading application, if these fields are not already stored in the Conduit data. Uses the get_cell_ids() and get_cell_node_ids() methods on StructuredBlock. * CI: Remove hdf5-1.8 testing * IOSS: Catalyst API 2 (sandialabs#463) * IOSS: Catalyst API 2 Added support to database for using my_processor and processor_count IOSS properties when operating in serial parallel mode and reading Conduit files. * IOSS: Catalyst API 2 Changed Conduit serial parallel test to use files from the one MPI rank can.ex2 test. --------- Co-authored-by: Greg Sjaardema <gsjaardema@gmail.com> * NEM_SPREAD: Add missing include file * CI: Enable netCDF quantize option * CI: Support newest netCDF; fmt * Enable new compression options * EXODUS: Fix non-hdf5 build * EXODUS: Do not commit the kluge to repository * IOSS: See if can fix spack ci build * CI: Spack build should not run on PR * CI: Spack build on master only [ci skip] * CI: Fix spack workflow syntax [ci skip] * IOSS: Add progress output to chain/face generation * IOSS: Reduce Face memory * EXODUS: Better control over exodus verbosity * EXODUS:, IOSS: Add bzip2 compression support * Further testing of non-lossy compression with plugins * CI: Update cmake-sems to work with current sems * Default netcdf should be 4.9.2 Signed-off-by: Greg Sjaardema <gsjaardema@gmail.com> * Clean up copyright dates Signed-off-by: Greg Sjaardema <gsjaardema@gmail.com> * Clean up copyright dates Signed-off-by: Greg Sjaardema <gsjaardema@gmail.com> * accidentally added to pr; removed Signed-off-by: Greg Sjaardema <gsjaardema@gmail.com> --------- Signed-off-by: Greg Sjaardema <gdsjaar@sandia.gov> Signed-off-by: Greg Sjaardema <gsjaardema@gmail.com> Co-authored-by: Greg Sjaardema <gdsjaar@sandia.gov> Co-authored-by: tjotaha <tjotaha@sandia.gov> Co-authored-by: Spiros Tsalikis <spiros.tsalikis@kitware.com>

tokusanya added 2 commits August 15, 2024 14:27

IOSS: More changes to support Dynamic Topology

feb7c5a

Merge remote-tracking branch 'upstream/master' into DynamicTopology-P…

ef77556

…haseIV

tokusanya commented Aug 15, 2024

View reviewed changes

gsjaardema requested changes Aug 15, 2024

View reviewed changes

tokusanya added 2 commits August 16, 2024 00:20

IOSS: Dynamic Topology changes based on code review

f273e85

IOSS:: More functonality to support Dynamic Topology

9f82422

gsjaardema requested changes Aug 19, 2024

View reviewed changes

tokusanya added 5 commits August 20, 2024 17:42

IOSS: Attempt to provide "change set" concept instead of "groups"

779e765

Merge remote-tracking branch 'upstream/master' into DynamicTopology-P…

e7c1db2

…haseIV

IOSS: Fix compiler errors

d1b1732

Merge remote-tracking branch 'upstream/master' into DynamicTopology-P…

14aef54

…haseIV

IOSS: Refactoring and unit test fix

90f97ec

gsjaardema reviewed Aug 27, 2024

View reviewed changes

tokusanya added 5 commits September 3, 2024 02:11

IOSS: Introduce concept of Change Sets

14be761

Merge remote-tracking branch 'upstream/master' into DynamicTopology-P…

4c21e0b

…haseIV

Hush compiler issues

2c71918

Fix for Dynamic Topology unit test

2791f05

Merge branch 'master' into DynamicTopology-PhaseIV

61236e4

gsjaardema requested changes Sep 9, 2024

View reviewed changes

gsjaardema reviewed Sep 10, 2024

View reviewed changes

packages/seacas/libraries/ioss/src/Ioss_Region.C Outdated Show resolved Hide resolved

gsjaardema reviewed Sep 10, 2024

View reviewed changes

packages/seacas/libraries/ioss/src/Ioss_Region.C Outdated Show resolved Hide resolved

gsjaardema reviewed Sep 10, 2024

View reviewed changes

packages/seacas/libraries/ioss/src/Ioss_Region.C Outdated Show resolved Hide resolved

tokusanya and others added 4 commits September 11, 2024 14:25

Hush more compiler warnings and address some review comments

2794a19

Merge branch 'master' into DynamicTopology-PhaseIV

1351b3c

Dynamic topology phase IV #481

Are you sure you want to change the base?

Dynamic topology phase IV #481

Conversation

tokusanya commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsjaardema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokusanya Aug 15, 2024 • 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

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

gsjaardema 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

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

gsjaardema 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

tokusanya Aug 15, 2024 •

edited

Loading