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

Add filter method to get.placement() #85

Open
wants to merge 8 commits into
base: devel
Choose a base branch
from

Conversation

mjchen1996
Copy link

Add filter methods for placement filtering: "all","max_lwr","max_pendant", "min_likelihood","lwr","pendant","likelihood"

@GuangchuangYu
Copy link
Member

为什么要把summarize_placement()删掉?

@GuangchuangYu GuangchuangYu requested a review from xiangpin August 19, 2022 09:17
@mjchen1996
Copy link
Author

为什么要把summarize_placement()删掉?

老师,我发现summarize_placement()调用了get.placement()是写死的,我想不到要怎么修改比较好,就先删掉了。

@GuangchuangYu
Copy link
Member

GuangchuangYu commented Aug 19, 2022

没想好,那就不要动,这不是显而易见的吗。

写死不是重点,重点是文件解析时,给出了基本的信息(这就可以是写死的一块),然后又有探索其它信息的可能性(这就是活的了)。

@mjchen1996
Copy link
Author

没想好,那就不要动,这不是显而易见的吗。

写死不是重点,重点是文件解析时,给出了基本的信息(这就可以是写死的一块),然后又有探索其它信息的可能性(这就是活的了)。

老师,是这样的,首先我在get.placement()里添加了其他的过滤方法,但我发现在summarize_placement() 调用get.placements()使用了固定的一种方法,by =="best"即为修改后的by == "min_likelihood",这样我添加的其他方法无法应用在summarize_placement() 中,进而无法应用在read.jplace()中,而我不想去改动read.jplace的参数。另外,也考虑到解析jplace文件后,通过get.placements()获取placement的信息,再传入data进去就可以开始下游的可视化了。

@GuangchuangYu
Copy link
Member

data哪是可以传来传去的。所谓封装就是让人看不见。

@GuangchuangYu
Copy link
Member

  1. 如果有必要的话,搞多个参数,也是可以的。
  2. 如果要像你说的那样,还把数据写回对象里,那应该是写replace method。就是说替换的操作是由一个replace method来完成的,替换了什么slot,对用户是不可见的。不要动不动就:来我告诉你,你用xx@xx然后怎么样怎么样。这是不对的。
  3. 你可以考虑其他的方式。

不管是什么样子的,首先是要想好,想清楚。不能说,我还没想好,所以我把代码注释掉一块。

如何呈现给用户是最主要的,至于怎么实现,都是次要的,你可以propose一些东西,我觉得OK了,你搞不了,让别的学生来搞也没问题。

最重要的一点,想清楚。想清楚。想清楚。

@mjchen1996
Copy link
Author

好的,老师,我再想想。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants