Skip to content

Conversation

@maki49
Copy link
Collaborator

@maki49 maki49 commented Oct 28, 2024

fix #5191

Finally I made io_cube depend on Pgrid.
I think this class is actually pretty well-designed (after a little modification). It can be made as the key structure of grid info. It encapsulates all the parallel communication (whose copy in write_cube have been deleted), while the serial version is just like a struct containing grid sizes.
With Pgrid, all those nxyz, bxyz, nplane, startz as well as the caller-side pw_rho and pw_big have been dropped.

In case of the file-comparison test booms, spin and efermi info are kept in write_cube.
However, the logic of iter to append seems strange: shouldn't the cube file and the grid info correspond one-to-one?

if (iter == 0)
{
ofs_cube.open(fn.c_str());
}
else
{
ofs_cube.open(fn.c_str(), std::ios::app);
}

@maki49 maki49 requested review from PeizeLin and kirk0830 October 28, 2024 18:11
@mohanchen mohanchen added the Refactor Refactor ABACUS codes label Oct 29, 2024
Copy link
Collaborator

@kirk0830 kirk0830 left a comment

Choose a reason for hiding this comment

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

thanks for your effort, however, as I have mentioned in issue #5191 , the function read_cube and write_cube themselves should not have any knowledge about how to handle grid parallelism.
For my preference, I prone not agree with ideas bundling many parameters together then pass it to a function that will not fully use it, it is not in aspect of waste but additional dependency risk.

@maki49
Copy link
Collaborator Author

maki49 commented Oct 29, 2024

According to the suggestion of @kirk0830 , I rewrite this PR in the the following way (coincident to the prefered option by @jinzx10 for item 1):

  • Add a new MPI-free read/write_cube function dealing with all the infomation of a cube file, stored by a data structure CubeInfo
  • Remove read_cube_core_match, read_cube_core_mismatch and write_cube_core()
  • Rename the previous read/write_cube as read/write_grid to run the read-assign-distribute / reduce-build-write workflow

@kirk0830
Copy link
Collaborator

According to the suggestion of @kirk0830 , I rewrite this PR in the the following way (coincident to the prefered option by @jinzx10 for item 1):

  • Add a new MPI-free read/write_cube function dealing with all the infomation of a cube file, stored by a data structure CubeInfo
  • Remove read_cube_core_match, read_cube_core_mismatch and write_cube_core()
  • Rename the previous read/write_cube as read/write_grid to run the read-assign-distribute / reduce-build-write workflow

@maki49 Hi, I think most of problems have been solved in a satisfying way, then may be the last one: I think the names read/write_grid are not clear enough. Will you prefer something like read/write_vdata_palgrid? in which palgrid stands for the parallized grid, and vdata for volumetric data.

@maki49
Copy link
Collaborator Author

maki49 commented Oct 30, 2024

Will you prefer something like read/write_vdata_palgrid? in which palgrid stands for the parallized grid, and vdata for volumetric data.

Make sense. I've renamed them.

Copy link
Collaborator

@kirk0830 kirk0830 left a comment

Choose a reason for hiding this comment

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

I have finished my review, if there is no further code changes.

This PR refactors the clean read/write_cube functions, allowing other developers to fully trust and use them as basic utility functions. The argument list of the read/write_cube functions will also not need to be modified for a considerable period of time, as the collection and distribution operations have been moved to its superior, the functions that call it.

From the principle of program design, there is still a concern that the Pgrid class object seems to be used to carry parallel information and contains broadcast and reduce methods for operating on charge density data. However, the object managing the charge density data (the Charge class object) does not have the capability to perform broadcast and reduce operations on its data, thus failing to achieve a cleaner abstraction, meaning that there will inevitably be coupling between Parallel_Grid and Charge based on the current class design.

However, eliminating the dependency on Parallel_Grid can be considered in stages for more things (but this has already exceeded the scope of this PR):

  1. (Noting that it is actually unnatural from the principle of code design to call a function named read_vdata_palgrid in the chg_init function. At the most basic level) while retaining the Parallel_Grid class and before implementing the methods within it as free functions, it is possible to consider constructing the Parallel_Grid object within a function that will perform charge density collection and distribution operations, at the cost of that function requiring all variables to construct the Parallel_Grid object.

  2. (Further) transform the collection and distribution operation functions into free functions, and then call them as needed (at this point, the dependency on the Parallel_Grid class has been removed).

  3. Store the data required for parallel distribution as attributes within the data management object, and let the data management object handle the collection and distribution of data (at this point, better encapsulation and abstraction can be achieved).

@mohanchen
Copy link
Collaborator

This ia an excellent example for discussions and actions among ABACUS developers.

@mohanchen mohanchen merged commit 705dad7 into deepmodeling:develop Oct 31, 2024
14 checks passed
@mohanchen mohanchen changed the title Refactor IO cube Refactor IO cube (Useful Information: excellent example for discussions and actions among ABACUS developers) Oct 31, 2024
@mohanchen mohanchen added the Useful Information Useful information for others to learn/study label Oct 31, 2024
@maki49 maki49 deleted the io_cube branch November 28, 2024 03:16
Fisherd99 pushed a commit to Fisherd99/abacus-BSE that referenced this pull request Mar 31, 2025
* remove the redundant info in read_cube

* remove redundant code in write_cube

* make Pgrid a serial-or-parallel grid class

* Pgrid contains nxyz

* [pre-commit.ci lite] apply automatic fixes

* rename reduce

* rewrite io_cube

* rename previous read/write_cube as read/write_grid

* remove CubeInfo and xyz-zxy permutation

* minor interface changes

* revert the call in rho_sym to fix SDFT failure

* rename r/w_grid as r/w_vdata_palgrid

* [pre-commit.ci lite] apply automatic fixes

* add rank parameter to pgrid.bcast to fix SDFT

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Refactor ABACUS codes Useful Information Useful information for others to learn/study

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Request: further refactor the cube file i/o functions

4 participants