Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OMEGA data types #19

Merged
merged 6 commits into from
Jun 20, 2023

Conversation

philipwjones
Copy link

@philipwjones philipwjones commented Jun 12, 2023

This is the OMEGA data types module (header file) that defines type aliases for standard data types, a generic real type and YAKL arrays for each supported type and up to 5 dimensions. It includes source code, user and develop documentation sections and a unit test.

The included unit test will be part of the CTest suite once the CMake build system is in place, but for now, this has been tested by manually building the unit test. On Chrysalis, this can be built with mpicxx and including the YAKL src and external directories and the omega/src/base directory in the include path. I have also tested on Crusher with the appropriate YAKL HIP flags. Note that for both of those tests, the latest version of YAKL was cloned since this branch still has an older version of YAKL.

Checklist

  • Documentation:
    • Design document has been generated and added to the docs
    • User's Guide has been updated
    • Developer's Guide has been updated
    • Documentation has been built locally and changes look as expected
  • Testing
    • A comment in the PR documents testing used to verify the changes including any tests that are added/modified/impacted.
    • CTest unit tests for new features have been added per the approved design.
    • Unit tests have passed. (Manually, no cdash yet)

Copy link

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments on the documentation.

I'll follow up separately on linting.

The code and test look great to me!

@xylar
Copy link

xylar commented Jun 13, 2023

@philipwjones, it would be helpful to have a discussion about linting. I'm not sure if we want to use this PR for that purpose or make a separate issue. Here's what I'm seeing.

  • We would need to disable most of the linting (clang-tidy, cppcheck and include-what-you-use) until we have CMake set up, so .pre-commit-config.yaml becomes:
      files: ^components/omega/
    repos:
      - repo: https://github.com/Takishima/cmake-pre-commit-hooks
        rev: v1.8.0
        hooks:
          - id: clang-format
          # - id: clang-tidy
          #   args: [--checks=readability-magic-numbers,--warnings-as-errors=*]
          # - id: cppcheck
          # - id: include-what-you-use
    
  • It seems like we will want a .clang-format file, something like:
    # We'll use defaults from the LLVM style, with 3 columns indentation.
    BasedOnStyle: LLVM
    IndentWidth: 3
    
    Language: Cpp
    AlignConsecutiveAssignments: true
    AlignConsecutiveBitFields: true
    AlignConsecutiveMacros:
      Enabled: true
      AcrossEmptyLines: true
      AcrossComments: false
    AlignEscapedNewlines: true
    AlignTrailingComments: true
    
  • I didn't see a way (see https://clang.llvm.org/docs/ClangFormatStyleOptions.html) to make clang-format happy with:
    • not having spaces after commas in multi-argument calls
    • not indenting nested for-loops

@philipwjones
Copy link
Author

Coincidentally, I had been looking at the linting output after I submitted the PR too. I do often like to align the = signs when we have several assignments grouped together so I'm ok with those mods in a clang-format config file. I'm not too concerned about the loop nesting - most loops should be indented. These were really tightly nested loops so indenting wasn't necessary but in practice, those loop forms will be converted to YAKL parallel_for so it won't be relevant. I'll go ahead and indent these.

I need to get my environments set up properly for both the doc build and the linting tools to catch some of these things before the PR...

@xylar
Copy link

xylar commented Jun 13, 2023

@philipwjones, for my part, what I did to test the docs and linting on my laptop was the following:

  • install Mambaforge
  • Create and activate a new environment
    mamba create -y -n omega_dev --file components/omega/dev-conda.txt
    mamba activate omega_dev
    
  • Build the docs:
    cd components/omega/doc
    make html
    google-chrome _build/html/index.html
    
  • Run the command that the linter is complaining about (removing the documentation files):
    pre-commit run --show-diff-on-failure --color=always components/omega/src/base/DataTypes.h components/omega/test/base/DataTypesTest.cpp
    

@philipwjones
Copy link
Author

Recent commits now pass the current (modified) linters and the doc builds successfully and looks fine.

Copy link

@xylar xylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! @philipwjones, it's up to you but presumably CI will pass if you do a rebase.

  - includes source header defining all supported OMEGA data types
  - includes user and developer documentation sections
  - includes a unit test that tests both the expected size of
    standard types and the creation of YAKL host and device arrays
@xylar
Copy link

xylar commented Jun 14, 2023

Thanks @philipwjones. It seems it's not happy with my .clang-format. Let's get this merged first and I can sort that out later.

@xylar xylar mentioned this pull request Jun 14, 2023
Copy link

@grnydawn grnydawn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me.

As this PR introduces the namespace OMEGA, I have one note about using OMEGA naming convention for Omega specific macros. In the Omega logging, I created several macros for logging such as OMEGA_LOG_INFO. it may be good to add these naming and namespace convention somewhere in Omega documentation.

Copy link

@mark-petersen mark-petersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@philipwjones philipwjones merged commit fb4eb16 into E3SM-Project:develop Jun 20, 2023
grnydawn pushed a commit to grnydawn/Omega that referenced this pull request Jun 23, 2023
* add OMEGA data types and YAKL arrays

  - includes source header defining all supported OMEGA data types
  - includes user and developer documentation sections
  - includes a unit test that tests both the expected size of
    standard types and the creation of YAKL host and device arrays
  - doxygen comments have been added
  - minor changes were made after review and linter results
@philipwjones philipwjones deleted the omega/data-types branch July 3, 2023 16:05
amametjanov pushed a commit that referenced this pull request Dec 5, 2023
activate snicar in icepack, check values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants