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

【Hackathon No.60】 #91

Closed
wants to merge 1 commit into from

Conversation

gsq7474741
Copy link
Contributor

add rfc of sparse_sqrt

@paddle-bot-old
Copy link

你的PR提交成功,感谢你对开源项目的贡献!
请检查PR提交格式和内容是否完备,具体请参考示例模版
Your PR has been submitted. Thanks for your contribution!
Please check its format and content. For this, you can refer to Template and Demo.

@gsq7474741
Copy link
Contributor Author

@TCChenlong merge pls

@tiancaishaonvjituizi
Copy link
Contributor

想请问一下 @zkh2016 ,approve 了同一个任务的两个不同的 rfc (另一个是 #90 ),是什么意图呢?

按照 hackathon 的规则只有一个最佳方案,大家要在最佳方案的基础上开发

黑客松评审组将在每个任务提交的所有有效方案中选择一个最佳方案,确认为本任务的 Leading Developer,并在开发方向上进行一定的指导,完善后的技术方案将会同步给所有任务提交小组,推荐大家基于最佳方案进行相应开发;

@tiancaishaonvjituizi
Copy link
Contributor

tiancaishaonvjituizi commented Apr 1, 2022

本 PR 提交的 rfc,完全照抄了同一作者的其它 rfc #74#75#76#77

然而本任务的 sqrt 是一个一元算子,不存在所谓的

主要参考scipy实现,将coo转换成csr再进行乘法,然后转换回coo

的说法。SciPy 的实现也完全不是这样。

在存在这种明显的事实错误的情况下,reviewer 仍然给了 approve。

此外再看同一作者的另一个 RFC:#77 ,同样存在着低级错误,却仍然被直接 approve,没有一条 review 意见:

调用路径为:paddle.sparse.divide
实现稀疏Tensor相加的功能。

Pytorch中有API ttorch.div

所以我认为相关的 reviewer 是没有尽到责任的。

再说一说这个参与者 @gsq7474741 ,他的多个 RFC 的内容高度雷同,存在多个低级错误,且在明知自己的 RFC 是两个 RFC 中较晚提交的那一个的情况下,at Paddle 开发者来 merge 他的 RFC,并在 RFC 未合并的情况下,抢先提交了实现 PR PaddlePaddle/Paddle#41272 ,我认为他是抱着浑水摸鱼的恶意的

希望 hackathon 组委会认真调查这件事情,是否仅仅是 reviewer 不负责任,还是涉及到特定 reviewer 和参与者之间的利益输送。以及如何处理相关的参与者和 reviewer,保证接下来的 hackathon 活动高质量进行,而不是变成吃相难看的闹剧。

@zkh2016
Copy link

zkh2016 commented Apr 1, 2022

想请问一下 @zkh2016 ,approve 了同一个任务的两个不同的 rfc (另一个是 #90 ),是什么意图呢?

按照 hackathon 的规则只有一个最佳方案,大家要在最佳方案的基础上开发

黑客松评审组将在每个任务提交的所有有效方案中选择一个最佳方案,确认为本任务的 Leading Developer,并在开发方向上进行一定的指导,完善后的技术方案将会同步给所有任务提交小组,推荐大家基于最佳方案进行相应开发;

你好,按照规则是需要按#90的方案进行开发的。

@TCChenlong
Copy link
Contributor

1、为什么对2个PR #90 #91 都 approve了:
因为 @zkh2016 刚开始因为被@,只注意到了#91,没有注意到 #90 ,先review了 #91 并 approve 了;后来经提醒,发现了 #90 ,开始 review #90 ,review 后认为也没有问题,就也approve了;review 结论是合入 #90 ,并告知 #91 ,但 TCChenlong 今天工作比较多,没来的及操作PR,故显示了2个都被 approve,但没有合入;

2、为什么存在事实错误,#77#91 却都 approve了,是否存在利益输送?

  • @gsq7474741@zkh2016 不认识,不存在利益输送的问题,请不要妄加猜测,给活动以及他人带来不好的影响。如有相关证据,请提供~
  • PR74-77,@zkh2016 读了第一个add,认为方案没有问题,且这四个API的逻辑很相似,且是同一作者,所以后面几个就大概看了一眼,没有注意其中的typo问题;
  • PR 91 也是因为是同一个作者,潜意识认为没什么问题,就直接通过了;
    @zkh2016 也说,因为没有仔细review,所以对这个工作失误很抱歉;

以上,本PR将close,#90 将 merge,任务60 以#90 中的方案为准。review 代码实现PR将按基于 #90 的方案开发的,提交先后顺序开始 review,如有问题,欢迎留言。

@TCChenlong TCChenlong closed this Apr 1, 2022
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