Skip to content

Comments

ENH: Covariance CPU SPMD support#3507

Draft
DDJHB wants to merge 3 commits intouxlfoundation:mainfrom
DDJHB:dev_cov_cpu_spmd
Draft

ENH: Covariance CPU SPMD support#3507
DDJHB wants to merge 3 commits intouxlfoundation:mainfrom
DDJHB:dev_cov_cpu_spmd

Conversation

@DDJHB
Copy link
Contributor

@DDJHB DDJHB commented Feb 9, 2026

Description

TODO


Checklist:

Completeness and readability

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes or created a separate PR with updates and provided its number in the description, if necessary.
  • Git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details).
  • I have resolved any merge conflicts that might occur with the base branch.

Testing

  • I have run it locally and tested the changes extensively.
  • All CI jobs are green or I have provided justification why they aren't.
  • I have extended testing suite if new functionality was introduced in this PR.

Performance

  • I have measured performance for affected algorithms using scikit-learn_bench and provided at least a summary table with measured data, if performance change is expected.
  • I have provided justification why performance and/or quality metrics have changed or why changes are not expected.
  • I have extended the benchmarking suite and provided a corresponding scikit-learn_bench PR if new measurable functionality was introduced in this PR.

@DDJHB
Copy link
Contributor Author

DDJHB commented Feb 9, 2026

/intelci: run

// Simple allreduce of centered crossproducts is incorrect because each
// rank uses its local mean. Un-center before allreduce, then re-center
// with global statistics after.
if (!desc.get_assume_centered()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it be simpler to use DAAL's Distributed<step2Master> algorithm for aggregation?
https://github.com/uxlfoundation/oneDAL/blob/main/samples/daal/cpp/mpi/sources/covariance_dense_distributed_mpi.cpp#L106

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting suggestion, will look into this

/// property value
compute_input(const table& data);

virtual ~compute_input() = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the reason of this change?
I see that it breaks the ABI, but were there any problems due to the lack of destructor definition in this and the partial_compute_input classes?

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.

2 participants