Add Tracers infrastructure #133

grnydawn · 2024-09-26T18:29:34Z

Created the Tracers class and implemented its methods. Unit tests and documentation were also added.

Tracer defintion file: "omega/src/ocn/TracerDefs.inc"
Tracer configuration file: "omega/configs/Default.yml"

Checklist

Documentation:
- Design document has been generated and added to the docs
- User's Guide has been updated
- Developer's Guide has been updated
Testing
- CTest unit tests for new features have been added per the approved design.

grnydawn · 2024-09-26T23:18:17Z

Revised several getXX functions to return an integer error code and to let the first argument as an output argument.

philipwjones

Thanks @grnydawn this looks good. Just a few comments that may require others to chime in on naming and having public indices.

philipwjones · 2024-09-27T16:47:43Z

components/omega/doc/devGuide/Tracers.md

+   const I4 TimeLevel,  ///< [in] Time level index
+   const I4 TracerIndex ///< [in] Global tracer index
+);
+


Update docs for the changes in retrieval functions

philipwjones · 2024-09-27T16:54:05Z

components/omega/src/ocn/Tracers.cpp

+//---------------------------------------------------------------------------
+std::string Tracers::packTracerFieldName(const std::string &TracerName) {
+   return "Tracer" + TracerName;
+}


I would prefer not to have the Tracer prefix on the field names. We want a consistent name between tracers, streams and wherever the field is referenced. So the tracer name is sufficient

Prefix is not used now, and packTracerFieldName function is removed.

philipwjones · 2024-09-27T16:55:32Z

components/omega/src/ocn/Tracers.cpp

+      TracerGroups[GroupName] =
+          std::pair<I4, I4>(GroupStartIndex, TracerIndex - GroupStartIndex);
+      FieldGroup::create("TracerGroup" + GroupName);
+   }


Same comment here - I would prefer not to have the TracerGroup prefix. Just use the tracer group name as the field group name so it will be the same in Tracers, Field, Streams, etc

The group prefix is not used now.

philipwjones · 2024-09-27T16:56:31Z

components/omega/src/ocn/Tracers.cpp

+   }
+
+// Read tracer definitions from file
+#include "TracerDefs.inc"


See my comment on tracer indices and the include file

@philipwjones is your suggestion somewhat equivalent to taking this part of the init routine and adding it to the include file? Or are you proposing to not iterate over the groups in the tracer config and just having hard coded indices for each tracer no matter if they are active or not?

@vanroekel Not hard-coded indices. We would still initialize at run time, just provide a shared named index that we could refer to rather than retrieving by name. But it looks like that's going to be more complicated than I thought so I think we'll stick with the current implementation for now.

Got it, so mostly moving the init to the .inc file, but I did see that Youngsung found it doesn't work as easily as possible. Thanks.

philipwjones · 2024-09-27T17:02:30Z

components/omega/src/ocn/TracerDefs.inc

+/// definitions selected in the Tracers section of the OMEGA YAML configuration
+/// file will be allocated into memory.
+//===----------------------------------------------------------------------===//
+


One of the reasons for the getByIndex function was to avoid the more expensive string comparison in getByName. In the current implementation, we would still need to get the index by name first and then use the index. I'm wondering whether in the include file, we could include a list of shared, public indices for each tracer (set when they are defined). Like IndxTemp, IndxSalt, IndxDebug1, etc. Since we only want to edit one file when adding tracers, that would mean putting the indices at the top of this include file and including it in Tracers.h. That means we would have to wrap all the defines in a defineAll function that is called where the current inc file is included.

What do others think?

@philipwjones , what are the benefits of having a list of shared and public indices for each tracers?

By way of explanation, this is what I'm thinking. The TracersDef.inc file would look like:

// Index defined for each tracer. The value of these tracer indices are still set in Tracers::init() and // tracers that have not been selected would have standard invalid value: Tracer::IndxInvalid = -1 static int IndxTemp; static int IndxSalt; static int IndxMyBGCTracer; static int IndxAnotherBGCTracer; static int IndxDebug1; static int IndxDebug2; etc. for all defined tracers // Tracer definitions packaged in a defineAllTracers function static void defineAllTracers() { // here we put all the define calls as they exist in the current TracersDef.inc... }

The TracersDef.inc file would then be included in the Tracers.h file in the public part of the Tracer class definition so that they are all available using eg Tracer::IndxSalt. In the Tracers.cpp file where the include currently sits, we would replace the include statement with a call to defineAllTracers. The init function would still assign consecutive indices to groups.

This would allow everyone to access the indices directly, providing speedier access without an explicit call to search for the index by name. I realize this is more of a Fortran style and not the usual C++ approach but I think it is both simpler and faster than a search by name or having all modules do the lookup on init.

This is just a proposal - others should weigh in...

@philipwjones, I tried to implement your suggested code in this branch (https://github.com/grnydawn/Omega/tree/ykim/omega/tracerinfra-def) but encountered a linking issue:

ld.lld: error: undefined symbol: OMEGA::Tracers::IndxTemp >>> referenced by Tracers.cpp >>> Tracers.cpp.o:(OMEGA::Tracers::defineAllTracers()) in archive ../src/libOmegaLib.a

To resolve the linker error, I added the following near the top of Tracers.cpp:

I4 Tracers::IndxTemp = 0; I4 Tracers::IndxSalt = 0;

This fix worked, but it makes me think that an additional user-provided include file might be necessary. I’m still learning C++, so I may be missing something.

That's right - forgot about that. The header just has the forward declaration but we still need to initialize in the implementation (.cpp) file. We might be able to include the same file in both with some creative ifdefs but this is starting to get more complicated than I had intended. Let me think about it some more

Let's stick with your initial implementation for now

I am bit late to this conversation, but if you declare the member index variables to be inline static then you can initialize them in the header file. Like this:

inline static int IndxTemp = -1;

Just to clarify, and echo a comment from @xylar at the last meeting: this is just a convenience/optimization that can be done for certain tracers and not a requirement, correct?

@mwarusz, thanks for the tip about using inline. I have updated Tracers with the inline, and it worked well.

@philipwjones, I am using the variable names for the static indexes from your example code above. If you need to add more tracer index variable names in "TracerDefs.inc", let me know, or we can update the file later.

hyungyukang · 2024-09-30T20:44:14Z

All ctest passed on pm-cpu.

I believe loading tracers from a file at initial time has not been implemented in this PR. I think we need this functionality for tracers as in OceanState, as many tests use tracer information from files (e.g., a cosine bell test in Polaris):

Omega/components/omega/src/ocn/OceanState.cpp

Line 51 in 48686a7

DefaultOceanState->loadStateFromFile(DefHorzMesh->MeshFileName, DefDecomp);

grnydawn · 2024-09-30T21:08:10Z

@hyungyukang, thank you for your review. We certainly need to add the feature of saving and loading the tracer to/from an external file within the tracer infrastructure. However, after discussions with the other members, we have decided to implement these features through IOStream. Therefore, the features will be added separately after this PR.

hyungyukang · 2024-10-01T02:15:24Z

@hyungyukang, thank you for your review. We certainly need to add the feature of saving and loading the tracer to/from an external file within the tracer infrastructure. However, after discussions with the other members, we have decided to implement these features through IOStream. Therefore, the features will be added separately after this PR.

Got it. Thanks for clarifying, @grnydawn !

mark-petersen · 2024-10-01T13:49:19Z

@grnydawn I tested on Frontier cpus and get the error below. I suspect I did something wrong in the compile or linking. Could you check? I'm at the airport and don't have time to look into it.

The error is:

27/27 Test #27: TRACERS_TEST .....................***Failed    2.45 sec
2: non-void function did not return a value
3: non-void function did not return a value
0: non-void function did not return a value
7: non-void function did not return a value
5: non-void function did not return a value
6: non-void function did not return a value
1: non-void function did not return a value
4: non-void function did not return a value
srun: error: frontier00637: tasks 1-7: Illegal instruction
srun: Terminating StepId=2609730.26
0: slurmstepd: error: *** STEP 2609730.26 ON frontier00637 CANCELLED AT 2024-10-01T09:41:08 ***
srun: error: frontier00637: task 0: Illegal instruction (core dumped)

Here are my steps:


######### Frontier cpu ############
CODE_DIR=opr
RUNDIR=241001_omega
mkdir /lustre/orion/cli115/scratch/mpetersen/runs/$RUNDIR
cd !$

cd /ccs/home/mpetersen/repos/E3SM/${CODE_DIR}
git submodule update --init --recursive externals/YAKL externals/ekat externals/scorpio cime
cd /lustre/orion/cli115/scratch/mpetersen/runs/$RUNDIR

module load cmake
rm -rf build
mkdir build
cd build
export PARMETIS_ROOT=$PROJWORK/cli115/pwjones/frontierlibs-cray/parmetis
cmake \
   -DOMEGA_CIME_COMPILER=crayclang \
   -DOMEGA_BUILD_TYPE=Release \
   -DOMEGA_CIME_MACHINE=frontier \
   -DOMEGA_PARMETIS_ROOT=${PARMETIS_ROOT}\
   -DOMEGA_BUILD_TEST=ON \
   -Wno-dev \
   -S /ccs/home/mpetersen/repos/E3SM/${CODE_DIR}/components/omega -B .
./omega_build.sh

# linking:
cd test
ln -isf ~/meshes/omega/O*nc .
cp /ccs/home/mpetersen/repos/E3SM/${CODE_DIR}/components/omega/configs/Default.yml omega.yml


salloc -A cli115 -J inter -t 2:00:00 -q debug -N 1 -S 0
RUNDIR=241001_omega
cd /lustre/orion/cli115/scratch/mpetersen/runs/$RUNDIR/build
./omega_ctest.sh

grnydawn · 2024-10-01T14:55:39Z

@mark-petersen , Indeed, there was an issue with not returning an error code. The Cray compiler caught this error, while the NVIDIA and Intel compilers did not. The issue is now fixed, and my test on Frontier has passed. Thank you very much for identifying this error.

xylar · 2024-10-01T19:57:05Z

@grnydawn, could you rebase onto develop so we can see if CI works again?

Created the Tracers class and implemented its methods. Unit tests and documentation were also added.

grnydawn · 2024-10-01T20:07:41Z

@xylar , thank you very much for the quick fix. I rebased onto develop and CI test has passed.

brian-oneill · 2024-10-01T20:18:50Z

Thanks so much for your work on this @grnydawn. Unit tests passed for me on Chrysalis and pm-cpu and pm-gpu (aside from an unrelated issue #143). This all looks good to me.

mwarusz

This looks good thanks @grnydawn. Aside from #143, everything passed for me on frontier. My only serious question is is about time level conventions.

mwarusz · 2024-10-01T22:22:45Z

components/omega/src/ocn/Tracers.cpp

+std::map<std::string, std::pair<I4, I4>> Tracers::TracerGroups;
+std::map<std::string, I4> Tracers::TracerIndexes;
+std::map<I4, std::string> Tracers::TracerNames;
+std::vector<std::string> Tracers::TracerDimNames = {"NCells", "NVertLevels"};


TracerDimNames is used only inside one function. Maybe it could be a local variable instead of a member ?

Yes, that’s better. I will move it to the local scope. Thanks!

mwarusz · 2024-10-01T22:38:55Z

components/omega/src/ocn/Tracers.cpp

+
+//---------------------------------------------------------------------------
+// get Tracer arrays
+// TimeLevel == [0:current, -1:previous, -2:two times ago, ...]


This convention for time levels looks different than what the state class uses. In there time levels are nonnegative, NextLevel = NTimeLevels - 1, and CurLevel = NTimeLevels - 2. Would it be possible to follow one convention ?

Thanks for bringing this issue up. I think it's worth some discussion because the difference in indexing conventions between Tracers and State stems from how the time level index is managed.

Both State and Tracers have a std::vector (let's call it ARR) as a private member in their respective classes to maintain multiple time levels.

In State, NextLevel is always set to the highest index of ARR, and CurLevel is NextLevel - 1. Since the ARR indices for the time levels are fixed, to increment the time level in State, the elements of ARR are swapped. I guess this approach may be better aligned with the established convention.

In Tracers, a private integer member (let's call it IDX) keeps track of the array (ARR) index for the current time level. To increment the time level, IDX is simply increased by 1. If IDX exceeds the size of ARR, it is reset to zero. I chose this approach to maximize performance because adding 1 is computationally cheap.

Both State and Tracers have a std::vector (let's call it ARR) as a private member in their respective classes to maintain multiple time levels.

Small correction, state stores its variables as a public Kokkos::Array<Array2DReal, MaxTimeLevels>.

I explained why I had suggested using Kokkos::Array for the State in the response to @sbrus89 comment below and why I no longer think it is necessary. As for the index convention, it is a separate issue, and I don't have a strong preference. One thing that would be nice is to have a common function for checking that a time index is valid.

@grnydawn and @mwarusz, after looking at this a little more, I think we should change State to follow the time index convention here (as well as using the std::vector approach).

@mwarusz , @sbrus89 , Agreed. We definitely need to have the same time level convention between State and Tracers. During my testing, I had to subtract (1) from the State time level to advance Tracers.

mark-petersen

Now passes on Frontier with crayclang compiler. Approving based on others comments above, and my own inspection.

sbrus89 · 2024-10-02T14:17:13Z

components/omega/src/ocn/Tracers.h

+   static std::vector<Array3DReal>
+       TracerArrays; ///< TimeLevels -> [Tracer, Cell, Vert]
+   static std::vector<HostArray3DReal>
+       TracerArraysH; ///< TimeLevels -> [Tracer, Cell, Vert]


In the State we (based on @mwarusz's suggestion) used:

Kokkos::Array<Array2DReal, MaxTimeLevels> LayerThickness;

to handle the time levels, to avoid any device incompatibility issues with std::vector. Is that something we would want to do here as well?

This was done to allow calling LayerThickness[TimeLevel] directly from a device kernel. However, I don't think we ended up using this capability anywhere. The approach taken in this PR is to have getter functions for tracer arrays that perform index checks and log errors. The logging alone prevents from calling them on GPU, but it is nice for safety. I think using std::vector here is fine and we should change the State to use it as well.

cbegeman · 2024-10-02T21:00:31Z

components/omega/doc/design/Tracers.md

+All other requirements will need to be tested in later system tests once
+I/O streams and time integration schemes have been implemented.


Should we be testing the validation of tracer fields using the min/max valid values or will this come later?

@philipwjones, do you have any thoughts on this?

It doesn't necessarily have to be a part of this PR, but it might be good to have a function like validate and/or validateAll to check tracer values for obvious errors like out-of-range. We would need a similar function in State. It might need to be up to the time integrator and/or a run-time variable to determine how frequently to check since it would impact performance. So maybe a later PR where we can decide how/when to call it?

grnydawn · 2024-10-03T15:48:14Z

@philipwjones, I have updated this to include the trace indices defined in "TracerDefs.inc," as @mwarusz suggested. I believe I have made the updates per the reviews except the recent validation feature. @hyungyukang, the basic algorithm of the tracer infrastructure has not changed, though some implementation details have been modified. Please feel free to review the updates and approve them when you think they are ready.

philipwjones

@grnydawn Thanks for the changes. I think a quick update to the documentation to reflect the addition of tracer indices in the include is needed and then I had a separate comment about removing undefined indices. With those changes, I'm fine with this and will approve on the assumption those changes are made.

philipwjones · 2024-10-03T17:14:42Z

components/omega/src/ocn/TracerDefs.inc

+inline static I4 IndxAnotherBGCTracer = Tracers::IndxInvalid;
+inline static I4 IndxDebug1           = Tracers::IndxInvalid;
+inline static I4 IndxDebug2           = Tracers::IndxInvalid;
+


remove the index entries that don't have a corresponding define function. That way the index list serves as a list of potentially available tracers.

Removed the unused entries.

grnydawn · 2024-10-03T19:57:21Z

@philipwjones , updated docs to add tracer indices in TracerDefs.inc file. Thanks!

hyungyukang

@grnydawn , thank you very much for your hard work on this PR. Codes look good to me. I have tested this PR with #132 and #140 simultaneously by implementing a cosine bell advection test case that requires 1) tracer array and info manipulations through this PR, 2) writing history output through IOStream PR, and 3) tracer advection through tracer tendency terms PR

During my testing, I had to separately read initial tracers from the initial file, but @grnydawn confirmed that this feature will be added to IOStreams separately. I have tested several functions in this PR, and they all worked fine.

In this cosine bell test, I disabled all tendency terms except for tracer advection.

tracer1 at t=0 day

tracer1 at t=24 days

Due to the rushed implementation of this test, there may be small bugs in the test. However, the functionality of this PR is working as intended

There was a concern about the time level convention for Tracers that differs from State. During testing, I had to subtract (1) from the State time level to advance Tracers. As @mwarusz and @sbrus89 mentioned, I believe this will be fixed soon.

I'm approving this PR based on my testing and the testing and visual inspection of others. Thanks again @grnydawn !

hyungyukang · 2024-10-04T14:18:49Z

components/omega/src/ocn/Tracers.cpp

+
+//---------------------------------------------------------------------------
+// get Tracer arrays
+// TimeLevel == [0:current, -1:previous, -2:two times ago, ...]


@mwarusz , @sbrus89 , Agreed. We definitely need to have the same time level convention between State and Tracers. During my testing, I had to subtract (1) from the State time level to advance Tracers.

cbegeman

@grnydawn Thanks for addressing my comments and for your hard work on this! Approving based on visual inspection and the testing of others.

sbrus89

These changes look great to me. I'll work on a PR to make the time handling consistent in State. Approving based on inspection and testing of others. Thanks your work on this @grnydawn!

sbrus89 · 2024-10-04T18:23:56Z

@philipwjones, I think this one is ready to merge, if you want to do the honors. I checked with Luke and he gave his go-ahead as well (but said he was having trouble with getting the PR to load so he could approve.)

vanroekel · 2024-10-04T19:25:45Z

components/omega/doc/userGuide/Tracers.md

+static void defineAllTracers() {
+
+   define("Temp",                            ///< [in] Name of tracer
+          "Potential Temperature",           ///< [in] Long name or description


noting here and in similar places, for TEOS-10 (new EOS) this should actually be Conservative Temperature not Potential Temperature and sea_water_conservative_temperature for the CF name

Similarkly salinity is now Absolute Salinity and not Practical Salinity units are g/kg instead of psu

This could be changed later.

vanroekel

Approving on visual inspection, but noting we'll need to update temperature and salinity naming and metadata at a future time to be consistent with TEOS-10. This can wait.

philipwjones · 2024-10-04T20:53:50Z

@vanroekel I'll make the change before merging, thanks.

- also added indices for defined debug tracers

grnydawn requested review from mark-petersen, mwarusz, hyungyukang and brian-oneill September 26, 2024 18:30

grnydawn assigned grnydawn and philipwjones and unassigned grnydawn Sep 26, 2024

grnydawn requested a review from philipwjones September 26, 2024 18:30

grnydawn force-pushed the ykim/omega/tracerinfra branch 2 times, most recently from 89bf794 to 30a52b9 Compare September 26, 2024 23:13

grnydawn force-pushed the ykim/omega/tracerinfra branch 2 times, most recently from 399ece2 to 2e30794 Compare September 27, 2024 13:52

philipwjones approved these changes Sep 27, 2024

View reviewed changes

grnydawn force-pushed the ykim/omega/tracerinfra branch from 2e30794 to 17af062 Compare September 28, 2024 17:22

grnydawn force-pushed the ykim/omega/tracerinfra branch from 17af062 to 57521d0 Compare October 1, 2024 14:51

grnydawn mentioned this pull request Oct 1, 2024

Github action of "lint with pre-commit and build docs (3.10)" has failed during "Setup Conda Environment" #141

Closed

Add Tracers infrastructure

3cfa5cf

Created the Tracers class and implemented its methods. Unit tests and documentation were also added.

grnydawn force-pushed the ykim/omega/tracerinfra branch from 57521d0 to 3cfa5cf Compare October 1, 2024 20:02

brian-oneill approved these changes Oct 1, 2024

View reviewed changes

mwarusz reviewed Oct 1, 2024

View reviewed changes

mark-petersen approved these changes Oct 2, 2024

View reviewed changes

sbrus89 reviewed Oct 2, 2024

View reviewed changes

cbegeman reviewed Oct 2, 2024

View reviewed changes

use inline static for tracer index variables

b2380b2

philipwjones approved these changes Oct 3, 2024

View reviewed changes

removes unused tracer variables; updates docs

6c7c8cf

Merge branch 'develop' into ykim/omega/tracerinfra

7ce7380

hyungyukang approved these changes Oct 4, 2024

View reviewed changes

cbegeman approved these changes Oct 4, 2024

View reviewed changes

sbrus89 approved these changes Oct 4, 2024

View reviewed changes

mwarusz approved these changes Oct 4, 2024

View reviewed changes

vanroekel reviewed Oct 4, 2024

View reviewed changes

vanroekel approved these changes Oct 4, 2024

View reviewed changes

Updated standard names for temp, salt tracers

c874146

- also added indices for defined debug tracers

philipwjones merged commit 8b50f3c into E3SM-Project:develop Oct 4, 2024
2 checks passed

hyungyukang mentioned this pull request Oct 9, 2024

Adds IOStream capabilities for omega input/output #132

Merged

10 tasks

		All other requirements will need to be tested in later system tests once
		I/O streams and time integration schemes have been implemented.

Add Tracers infrastructure #133

Add Tracers infrastructure #133

Conversation

grnydawn commented Sep 26, 2024 • edited by hyungyukang Loading

grnydawn commented Sep 26, 2024 • edited Loading

philipwjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grnydawn Sep 28, 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

philipwjones Sep 27, 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

mwarusz Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyungyukang commented Sep 30, 2024 • edited Loading

grnydawn commented Sep 30, 2024

hyungyukang commented Oct 1, 2024

mark-petersen commented Oct 1, 2024

grnydawn commented Oct 1, 2024

xylar commented Oct 1, 2024

grnydawn commented Oct 1, 2024

brian-oneill commented Oct 1, 2024

mwarusz 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

mwarusz Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark-petersen 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

grnydawn commented Oct 3, 2024

philipwjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grnydawn commented Oct 3, 2024

hyungyukang left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbegeman left a comment

Choose a reason for hiding this comment

sbrus89 left a comment

Choose a reason for hiding this comment

sbrus89 commented Oct 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanroekel left a comment

Choose a reason for hiding this comment

philipwjones commented Oct 4, 2024

grnydawn commented Sep 26, 2024 •

edited by hyungyukang

Loading

grnydawn commented Sep 26, 2024 •

edited

Loading

grnydawn Sep 28, 2024 •

edited

Loading

philipwjones Sep 27, 2024 •

edited

Loading

mwarusz Oct 1, 2024 •

edited

Loading

hyungyukang commented Sep 30, 2024 •

edited

Loading

mwarusz Oct 2, 2024 •

edited

Loading

hyungyukang left a comment •

edited

Loading