Skip to content

[Base] move DistributedDataShared to standalone header & add tests#1169

Merged
tdavidcl merged 7 commits intoShamrock-code:mainfrom
tdavidcl:ddsharedrework
Aug 12, 2025
Merged

[Base] move DistributedDataShared to standalone header & add tests#1169
tdavidcl merged 7 commits intoShamrock-code:mainfrom
tdavidcl:ddsharedrework

Conversation

@tdavidcl
Copy link
Member

@tdavidcl tdavidcl commented Aug 9, 2025

No description provided.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @tdavidcl, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

I've refactored the DistributedDataShared class by moving it to its own standalone header file to improve code organization and maintainability. This change also allowed for the addition of a dedicated and comprehensive suite of unit tests for the class, ensuring its reliability. Necessary include path updates have been made across the affected files.

Highlights

  • Code Refactoring: The DistributedDataShared class has been extracted from shambase/DistributedData.hpp and moved into its own dedicated header file, shambase/DistributedDataShared.hpp. This improves modularity and organization within the codebase.
  • New Unit Tests: A new, comprehensive unit test file, src/tests/shambase/DistributedDataShared.cpp, has been added. This file includes extensive tests for all core functionalities of the DistributedDataShared class, ensuring its correctness and stability.
  • Dependency Updates: Updated include paths across various files (distributedDataComm.hpp, GhostZoneData.hpp in both ramses and zeus, and GhostZones.cpp) to reflect the new location of the DistributedDataShared header.
  • Minor Code Cleanup: A minor refactoring in shambase/DistributedData.hpp involved replacing logger::raw_ln with print_ln, along with the removal of the shamcomm/logs.hpp include, streamlining the printing mechanism.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request successfully refactors the DistributedDataShared class into its own header file, improving code organization. New unit tests have been added, which is great for ensuring correctness. My review focuses on the implementation of DistributedDataShared, with suggestions to improve performance and maintainability by using modern C++ features more effectively, such as templated callables over std::function and more efficient map iteration.

@github-actions
Copy link
Contributor

Workflow report

workflow report corresponding to commit cc10aa7
Commiter email is timothee.davidcleris@proton.me
GitHub page artifact URL GitHub page artifact link (can expire)

Pre-commit check report

Pre-commit check: ✅

trim trailing whitespace.................................................Passed
fix end of files.........................................................Passed
check for merge conflicts................................................Passed
check that executables have shebangs.....................................Passed
check that scripts with shebangs are executable..........................Passed
check for added large files..............................................Passed
check for case conflicts.................................................Passed
check for broken symlinks................................................Passed
check yaml...............................................................Passed
detect private key.......................................................Passed
No-tabs checker..........................................................Passed
Tabs remover.............................................................Passed
Validate GitHub Workflows................................................Passed
clang-format.............................................................Passed
black....................................................................Passed
ruff check...............................................................Passed
Check doxygen headers....................................................Passed
Check license headers....................................................Passed
Check #pragma once.......................................................Passed
Check SYCL #include......................................................Passed
No ssh in git submodules remote..........................................Passed

Test pipeline can run.

Clang-tidy diff report

No relevant changes found.
Well done!

You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review.

Doxygen diff with main

Removed warnings : 44
New warnings : 35
Warnings count : 7021 → 7012 (-0.1%)

Detailed changes :
- src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:121: warning: The following parameter of shamalgs::collective::fetch_all_storeload(shambase::DistributedData< T > &src, std::vector< P > local_ids, std::vector< P > global_ids, std::function< u64(P)> id_getter) is not documented:
+ src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:122: warning: The following parameter of shamalgs::collective::fetch_all_storeload(shambase::DistributedData< T > &src, std::vector< P > local_ids, std::vector< P > global_ids, std::function< u64(P)> id_getter) is not documented:
- src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:34: warning: Member SerializedDDataComm (typedef) of namespace shamalgs::collective is not documented.
+ src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:35: warning: Member SerializedDDataComm (typedef) of namespace shamalgs::collective is not documented.
- src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:36: warning: Member distributed_data_sparse_comm(std::shared_ptr< sham::DeviceScheduler > dev_sched, SerializedDDataComm &send_ddistrib_data, SerializedDDataComm &recv_distrib_data, std::function< i32(u64)> rank_getter, std::optional< SparseCommTable > comm_table={}) (function) of namespace shamalgs::collective is not documented.
+ src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:37: warning: Member distributed_data_sparse_comm(std::shared_ptr< sham::DeviceScheduler > dev_sched, SerializedDDataComm &send_ddistrib_data, SerializedDDataComm &recv_distrib_data, std::function< i32(u64)> rank_getter, std::optional< SparseCommTable > comm_table={}) (function) of namespace shamalgs::collective is not documented.
- src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:44: warning: Member serialize_sparse_comm(std::shared_ptr< sham::DeviceScheduler > dev_sched, shambase::DistributedDataShared< T > &&send_distrib_data, shambase::DistributedDataShared< T > &recv_distrib_data, std::function< i32(u64)> rank_getter, std::function< sham::DeviceBuffer< u8 >(T &)> serialize, std::function< T(sham::DeviceBuffer< u8 > &&)> deserialize, std::optional< SparseCommTable > comm_table={}) (function) of namespace shamalgs::collective is not documented.
+ src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:45: warning: Member serialize_sparse_comm(std::shared_ptr< sham::DeviceScheduler > dev_sched, shambase::DistributedDataShared< T > &&send_distrib_data, shambase::DistributedDataShared< T > &recv_distrib_data, std::function< i32(u64)> rank_getter, std::function< sham::DeviceBuffer< u8 >(T &)> serialize, std::function< T(sham::DeviceBuffer< u8 > &&)> deserialize, std::optional< SparseCommTable > comm_table={}) (function) of namespace shamalgs::collective is not documented.
- src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:90: warning: The following parameter of shamalgs::collective::fetch_all_simple(shambase::DistributedData< T > &src, std::vector< P > local_ids, std::vector< P > global_ids, std::function< u64(P)> id_getter) is not documented:
+ src/shamalgs/include/shamalgs/collective/distributedDataComm.hpp:91: warning: The following parameter of shamalgs::collective::fetch_all_simple(shambase::DistributedData< T > &src, std::vector< P > local_ids, std::vector< P > global_ids, std::function< u64(P)> id_getter) is not documented:
- src/shambase/include/shambase/DistributedData.hpp:291: warning: Member get_native() (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:293: warning: Member add_obj(u64 left_id, u64 right_id, T &&obj) (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:298: warning: Member for_each(std::function< void(u64, u64, T &)> &&f) (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:304: warning: Member tranfer_all(std::function< bool(u64, u64)> cd, DistributedDataShared &other) (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:323: warning: Member has_key(u64 left_id, u64 right_id) (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:327: warning: Member get_element_count() (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:330: warning: Member map(std::function< Tmap(u64, u64, T &)> map_func) (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:338: warning: Member reset() (function) of class shambase::DistributedDataShared is not documented.
- src/shambase/include/shambase/DistributedData.hpp:340: warning: Member is_empty() (function) of class shambase::DistributedDataShared is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:34: warning: Member Tscal (typedef) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:35: warning: Member Tgridscal (typedef) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:35: warning: Member Tscal (typedef) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:36: warning: Member Tgridscal (typedef) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:36: warning: Member dim (variable) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:37: warning: Member dim (variable) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:38: warning: Compound shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:39: warning: Compound shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:39: warning: Member offset (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:40: warning: Member offset (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:40: warning: Member periodicity_index (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:41: warning: Member periodicity_index (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:41: warning: Member volume_target (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:42: warning: Member volume_target (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:44: warning: Compound shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:45: warning: Compound shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:45: warning: Member build_infos (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:46: warning: Member build_infos (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:46: warning: Member ids_interf (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:47: warning: Member cell_count_ratio (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:47: warning: Member ids_interf (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:48: warning: Member cell_count_ratio (variable) of struct shammodels::basegodunov::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:50: warning: Member GeneratorMap (typedef) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:51: warning: Member GeneratorMap (typedef) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:51: warning: Member ghost_gen_infos (variable) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:52: warning: Member ghost_gen_infos (variable) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:53: warning: Member ghost_id_build_map (variable) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:54: warning: Member ghost_id_build_map (variable) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:56: warning: Member build_interface_native(std::function< T(u64, u64, InterfaceBuildInfos, sycl::buffer< u32 > &, u32)> fct) (function) of class shammodels::basegodunov::GhostZonesData is not documented.
+ src/shammodels/ramses/include/shammodels/ramses/GhostZoneData.hpp:57: warning: Member build_interface_native(std::function< T(u64, u64, InterfaceBuildInfos, sycl::buffer< u32 > &, u32)> fct) (function) of class shammodels::basegodunov::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:34: warning: Member Tscal (typedef) of class shammodels::zeus::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:35: warning: Member Tgridscal (typedef) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:35: warning: Member Tscal (typedef) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:36: warning: Member Tgridscal (typedef) of class shammodels::zeus::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:36: warning: Member dim (variable) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:37: warning: Member dim (variable) of class shammodels::zeus::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:38: warning: Compound shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:39: warning: Compound shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:39: warning: Member offset (variable) of struct shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:40: warning: Member offset (variable) of struct shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:40: warning: Member periodicity_index (variable) of struct shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:41: warning: Member periodicity_index (variable) of struct shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:41: warning: Member volume_target (variable) of struct shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:42: warning: Member volume_target (variable) of struct shammodels::zeus::GhostZonesData::InterfaceBuildInfos is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:44: warning: Compound shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:45: warning: Compound shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:45: warning: Member build_infos (variable) of struct shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:46: warning: Member build_infos (variable) of struct shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:46: warning: Member ids_interf (variable) of struct shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:47: warning: Member cell_count_ratio (variable) of struct shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:47: warning: Member ids_interf (variable) of struct shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:48: warning: Member cell_count_ratio (variable) of struct shammodels::zeus::GhostZonesData::InterfaceIdTable is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:50: warning: Member GeneratorMap (typedef) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:51: warning: Member GeneratorMap (typedef) of class shammodels::zeus::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:51: warning: Member ghost_gen_infos (variable) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:52: warning: Member ghost_gen_infos (variable) of class shammodels::zeus::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:53: warning: Member ghost_id_build_map (variable) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:54: warning: Member ghost_id_build_map (variable) of class shammodels::zeus::GhostZonesData is not documented.
- src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:56: warning: Member build_interface_native(std::function< T(u64, u64, InterfaceBuildInfos, sycl::buffer< u32 > &, u32)> fct) (function) of class shammodels::zeus::GhostZonesData is not documented.
+ src/shammodels/zeus/include/shammodels/zeus/GhostZoneData.hpp:57: warning: Member build_interface_native(std::function< T(u64, u64, InterfaceBuildInfos, sycl::buffer< u32 > &, u32)> fct) (function) of class shammodels::zeus::GhostZonesData is not documented.

@tdavidcl tdavidcl merged commit bc0bfbb into Shamrock-code:main Aug 12, 2025
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant