Skip to content

Conversation

@francois-kitware
Copy link

@francois-kitware francois-kitware commented Nov 8, 2024

Add the catalyst plugin developed by Kitware as an official PDI plugin.

The plugin code have been copied in the branch and the catalyst library dependency is added as a submodule for easy update in the future.

When built with BUILD_TESTING CMake variable, the pdi-catalyst-plugin tests should pass:

$ ctest -R Catalyst                                             
Test project /home/francois/Documents/Numpex/PDI/github/build                                  
    Start 1417: TestPDICatalyst  
1/2 Test #1417: TestPDICatalyst ..................   Passed    3.18 sec                        
    Start 1418: TestPDICatalystMPI
2/2 Test #1418: TestPDICatalystMPI ...............   Passed    1.61 sec 

Looks like lots of boxes below are not checked. I'll update everything after PDI's team code review.

François

List of things to check before making a PR

Before merging your code, please check the following:

  • you have added a line describing your changes to the Changelog;
  • you have added unit tests for any new or improved feature;
  • In case you updated dependencies, you have checked pdi/docs/CheckList.md
  • you have checked your code format:
    • you have checked that you respect all conventions specified in CONTRIBUTING.md;
    • you have checked that the indentation and formatting conforms to the .clang-format;
    • you have documented with doxygen any new or changed function / class;
  • you have correctly updated the copyright headers:
    • your institution is in the copyright header of every file you (substantially) modified;
    • you have checked that the end-year of the copyright there is the current one;
  • you have updated the AUTHORS file:
    • you have added yourself to the AUTHORS file;
    • if this is a new contribution, you have added it to the AUTHORS file;
  • you have added everything to the user documentation:
    • any new CMake configuration option;
    • any change in the yaml config;
    • any change to the public or plugin API;
    • any other new or changed user-facing feature;
    • any change to the dependencies;
  • you have correctly linked your MR to one or more issues:
    • your MR solves an identified issue;
    • your commit contain the Fix #issue keyword to autoclose the issue when merged.

@jbigot jbigot linked an issue Nov 27, 2024 that may be closed by this pull request
Copy link
Member

@jbigot jbigot left a comment

Choose a reason for hiding this comment

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

Can you also add Kitware to the copyright at the top of the root CMakeLists.txt you contributed to

@jbigot
Copy link
Member

jbigot commented Dec 11, 2024

Thank you so much for the contribution @francois-kitware !

From our point of view, it's basically good to go as-is, if you maintain the plugin separately in a submodule. We just need to add a Changelog entry, and some other minor elements related to our "checkboxes".

Would you also be interested in the review of the plugin code? In this case, this should be done on your gitlab since from github we only have a submodule reference.

#include <assert.h>
#include <math.h>
#include <stdio.h>
#include <stdlib.h>
Copy link
Author

@francois-kitware francois-kitware Nov 7, 2025

Choose a reason for hiding this comment

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

I must add #include <stdint.h> to build from source, otherwise basic types like uint8_t are unknown.

Copy link
Member

@benoitmartin88 benoitmartin88 Nov 14, 2025

Choose a reason for hiding this comment

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

It's probably better to include cstdint instead.

project(pdi_catalyst_plugin LANGUAGES C CXX)

find_package(PDI REQUIRED COMPONENTS plugins)
find_package(Catalyst REQUIRED)
Copy link
Author

Choose a reason for hiding this comment

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

There is a mismatch in the package name. The main PDI CMakeLists.txt needs catalyst_DIR variable whereas the plugin ask for Catalyst_DIR (with a capital C at the beginning).

A quick solution is to change this line to find_package(catalyst REQUIRED)

configure_file(config_init.yml config.yml)

find_package(Python3 COMPONENTS Interpreter)
add_test(NAME Test_tuto_september_2025_ghost_attributes COMMAND ${Python3_EXECUTABLE} "${CMAKE_SOURCE_DIR}/test_tuto_september_2025_ghost_attributes/run_test.py" "${CMAKE_BINARY_DIR}/test_tuto_september_2025_ghost_attributes/" "${CMAKE_SOURCE_DIR}/test_tuto_september_2025_ghost_attributes/")
Copy link
Author

Choose a reason for hiding this comment

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

What is the purpose of this test?
I would suggest to move it to the test folder, add the "Catalyst" word in the name and maybe remove the date reference?

Copy link
Contributor

Choose a reason for hiding this comment

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

The code in this directory correspond to a demo given in a PDI tutorial.
The purpose of this code, it is to show how to introduce ghost cells.
Perhaps, it is better to create a repository example?

Copy link
Member

Choose a reason for hiding this comment

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

This should probably be moved to the tutorial repo.

env["CATALYST_DATA_DUMP_DIRECTORY"] = binary_folder
env["PDI_PLUGIN_PATH"] = binary_folder + '/..'

result = subprocess.run([binary_folder + "/Test_tuto_september_2025", binary_folder + "/ex6.yml"], env=env)
Copy link
Author

Choose a reason for hiding this comment

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

Where is ex6.yml file? I can't find it in this plugin folder.

Copy link
Contributor

@jmorice91 jmorice91 Nov 10, 2025

Choose a reason for hiding this comment

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

Thank for the comment, this file correspond to config.yml in old version of previous commit

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.

Plugin Catalyst

5 participants