Skip to content

Conversation

@cindytsai
Copy link
Collaborator

@cindytsai cindytsai commented Feb 19, 2025

Refactoring the Ugly Stuff

This pr doesn't change any API, only the underlying codes are refactored.

Enhancement

Pybind11

Directly using pybind11 to build libyt Python module and bind data.
Even though I have added pybind11, I still keep the original version build using pure Python C API.

Note

What I have learned from this is to use Pybind11 to bind meta data, for example, building a dictionary, a list, etc. But for building a Python module, building a large dictionary, wrapping data to a NumPy array, using pure Python C API is just better.

Refactoring

  • Separate different operations into either namespaces or classes.

Magic Command and Defined Command

I have merged these two classes.
Though there is still some space for improvement, I still think the definitions inside the new class are strange, I think it is ok to leave it for future, when coupling other well-defined ipython magic command.

Function Status and Function Status List

Refactor it to match the naming convention. Probably should decouple collective functions from local functions.

Libyt Process Control

Move all globals into this class.
It also holds different classes that governs different modules.

CommMpiRma

This new class handles the one-sided MPI operation. It separates the data to be distributed and mpi metadata, so that the class is agnostic to distributed data.

To use it, we need to inherit CommMpiRma<T> and implement corresponding functions (GetDataSize and GetDataLen).
AMR grid and particle data use this.

DataStructureAmr

This class deals with everything about AMR data structure, including:

  • Initializing and deconstructing the storage
  • Gathering hierarchy
  • Binding info and data to Python
  • Checking data

DataHub

Data hub is a temporary storage for generated or gathered data.
And once the class is deconstructed, it will free them based on whether this is a new allocation and whether the ownership is taken.

Unit Testing

  • Using googletest to do unit test
  • Since some of them are still coupled with MPI, I have to separate them to parallel and serial. (This is because decoupling them might change the API, and I don't want that for now.)
MPI Serial
General MPI, Big MPI, RMA
numpy_controller numpy_controller
LibytPythonShell (with MPI) LibytPythonShell
DataStructureAmr (with MPI) DataStructureAmr

Memory Profiling

A new GitHub Action for doing memory profile using valgrind is added. The profile will be in artifacts.

GitHub Action

GitHub Action environment, ex: os (linux or macOS), Python version (3.7 ~ 3.14), MPI implementation (openmpi or mpich), different building options, are done in the way that different combination of these are covered.

Because some of them are orthogonal, we can separate them.

Doc

Setup doxygen and breathe and show the index page. (Though this overlapped with the already documented libyt API, it will help us document the code instead of writing it again in the future.

Formatting

clang-format

In the previous release, column limit 120 and indentation 4 were used.
This is taking TOO much space, so I changed to column limit 90 and indentation 2.

Changing the code formatting is not going to happen again.

cmake-format

Format cmake file. The configuration is in cmake-format.json.

TODO

  • Bump version number.
    • doc (conf.py, index.md)
    • Cmake (CMakeLists.txt)
    • doxygen (Doxyfile)
    • libyt.h
  • Add the badge and update readme.
  • Also PR to yt_libyt and jupyter_libyt.
  • Update doc for jupyter-libyt
  • Should make amr example test run also run -DUSE_PYBIND11.
  • Read through the file.

…rking.

While example can run successfully, the unit test failed at numpy part.
I think this is because the initialization of numpy cannot be shared between the one in libyt library and the one initialized in googletest. Because I can successfully called numpy c api in unit test, but failed in libyt.
This is because NumPy API initialization only imports the api within a translation unit.
Since we compile it to a libyt library and make unit test link to it, it is in separate translation unit, we need to have a public API to initialize NumPy API within libyt library itself.
(https://stackoverflow.com/questions/32899621/numpy-capi-error-with-import-array-when-compiling-multiple-modules/35362918)
This also raise a question of should I separate Mpi operation in DS class?
Though this fix it, I better not called it here, maybe I should singled out numpy function to a class.
Again, this is due to numpy api initialization.
cindytsai and others added 26 commits February 18, 2025 21:43
…der it.

Some part is still not working, but at least breathe can read it.
Since I haven't update all the doc to be able to parse by doxygen, I'm skipping this now. And will update it in the future.
So currently the doxygen doc won't render in the web page.
This will solve the problem, but I don't think I have time to update every doc. Future work here.
I'm not showing api doc in the website anyway.
This is just a test.
though nasty but at least it is showing something.
Testing breathe purpose. I don't think I should use the doxygen generate api doc just yet.
updates:
- [github.com/pre-commit/mirrors-clang-format: v18.1.8 → v19.1.7](pre-commit/mirrors-clang-format@v18.1.8...v19.1.7)
@cindytsai cindytsai merged commit 6e66b6e into yt-project:main Mar 7, 2025
32 checks passed
@cindytsai cindytsai deleted the Refactor branch March 7, 2025 04:19
@cindytsai cindytsai mentioned this pull request Jun 8, 2025
6 tasks
cindytsai added a commit to cindytsai/libyt that referenced this pull request Jun 18, 2025
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.

1 participant