Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ant project-Jacobi-EVD and Jacobi-PCA #950

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fdddy
Copy link

@fdddy fdddy commented Dec 26, 2024

Pull Request

What problem does this PR solve?

[Feature] Add a new eigendecomposition method called Jacobi and the PCA implement based on Jacobi-evd

Copy link


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


fdddy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@deadlywing deadlywing self-requested a review December 30, 2024 02:24
Copy link

github-actions bot commented Feb 1, 2025

Stale pull request message. Please comment to remove stale tag. Otherwise this pr will be closed soon.

@github-actions github-actions bot closed this Feb 8, 2025
@deadlywing deadlywing reopened this Feb 8, 2025
@Candicepan Candicepan closed this Feb 8, 2025
@Candicepan Candicepan reopened this Feb 8, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
@secretflow secretflow unlocked this conversation Feb 11, 2025
Copy link
Contributor

Choose a reason for hiding this comment

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

不要更改这个文件哈~

Copy link
Contributor

Choose a reason for hiding this comment

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

用buildfier格式化一下,参考这里,后同

@@ -0,0 +1,16 @@


load("@rules_python//python:defs.bzl", "py_library")
Copy link
Contributor

Choose a reason for hiding this comment

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

用SPU里定义的spu_py_xxx,后同

# ONLY test small matrix for usage purpose
n = 10
mat = jnp.array(np.random.rand(n, n))
mat = (mat + mat.T) / 2
Copy link
Contributor

Choose a reason for hiding this comment

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

这个方法只适用于对称矩阵么?


return index_pairs

def serial_jacobi_evd(
Copy link
Contributor

Choose a reason for hiding this comment

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

麻烦增加一些必要的注释信息,起码需要包括:

  1. 明文下原始算法名称(最好包括ref)
  2. (optional)若有与明文算法不同处,可以简单概括并说明一下
  3. 涉及的参数介绍 以及 返回值介绍

import jax.numpy as jnp
import numpy as np

import spu.spu_pb2 as spu_pb2 # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

import spu.spu_pb2 as spu_pb2 is not valid anymore, use import spu.libspu as libspu instead, you can refer to the sml/cluster/tests/kmeans_test.py


J_combined = J.copy()
for i in range(len(pair)):
if(mask.at[i]):
Copy link
Contributor

Choose a reason for hiding this comment

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

这里的mask是 public value?

for i in range(len(pair)):
if(mask.at[i]):
k, l = ks[i], ls[i]
J_combined = J_combined.at[k, k].set(c[i]).at[l, l].set(c[i]).at[k, l].set(s[i]).at[l, k].set(-s[i])
Copy link
Contributor

Choose a reason for hiding this comment

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

rows: [k,l,k,l], cols: [k,l,l,k],vals: [c,c,s,-s]
J_combined.at[rows, cols].set(vals)

可以试一试把多个.at .set 合并成一个

Copy link
Contributor

Choose a reason for hiding this comment

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

所有python文件也需要格式化一下,参考上面的链接

@deadlywing
Copy link
Contributor

sign 一下 CLA哈~

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.

4 participants