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.

Sign up for GitHub

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

Jump to bottom

polish README #195

Merged
merged 1 commit into from
Jun 9, 2019
Merged

polish README #195

merged 1 commit into from
Jun 9, 2019

Conversation

Roger-luo
Copy link
Member

Polish the README to make it more native based on @ViralBShah's suggestions. Thanks BTW.

@codecov
Copy link

codecov bot commented Jun 6, 2019 •
edited
Loading

Codecov Report

Merging #195 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #195   +/-   ##
=====================================
  Coverage      20%    20%           
=====================================
  Files           2      2           
  Lines           5      5           
=====================================
  Hits            1      1           
  Misses          4      4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2235804...ee7099c. Read the comment docs.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1204

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 1201: 0.0%
Covered Lines: 0
Relevant Lines: 4
💛 - Coveralls

3 similar comments
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1204

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 1201: 0.0%
Covered Lines: 0
Relevant Lines: 4
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1204

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 1201: 0.0%
Covered Lines: 0
Relevant Lines: 4
💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1204

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 1201: 0.0%
Covered Lines: 0
Relevant Lines: 4
💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 6, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 1205

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%
Totals Coverage Status
Change from base Build 1201: 0.0%
Covered Lines: 0
Relevant Lines: 4
💛 - Coveralls

@Roger-luo
Copy link
Member Author

@GiggleLiu good to merge?

GiggleLiu
GiggleLiu reviewed Jun 6, 2019
View reviewed changes
README.md
Our design enables:

* Hierarchical design of quantum algorithms and greater abstraction for quantum circuits
* Heterogeneous computing
Copy link
Member

Choose a reason for hiding this comment

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

What is exactly heterogeneous computing?

Copy link
Member Author

@Roger-luo Roger-luo Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

heterogeneous means it can run on different hardware.

The design of block IR is hardware free. It enables heterogeneous computing.

I could add a link to wiki for this.

Copy link
Member

@GiggleLiu GiggleLiu Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Why not remove it if it requres wiki, it is not the key feature. Do you mean GPU backend? Then just say GPU support.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is not only about GPU support, it is about the why is the design hardware free, what does this hardware free capable of. Heterogeneous is conceptually a more powerful and precise description of hardware free.

Copy link
Member Author

@Roger-luo Roger-luo Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Simply say it is GPU support cannot cover other work in YaoExperiment, YaoGraphs, and other possible direction that other people are interested in (like real hardware support in Regetti). Thus heterogeneous is better, if you think this needs some description, we could add a link. But you can't get rid of this concept if we are still dealing with this arch, and this is a very common feature among many softwares.

This concept has been existing for decades, if people don't understand it, they should read wiki and learn it.

Copy link
Member

Choose a reason for hiding this comment

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

Most framework supports hardware... I mean this is not a unique feature. Also, YaoExperiment has only basic support.

Copy link
Member Author

@Roger-luo Roger-luo Jun 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, but I mean even it is basic, we have it, and we also have RBNF for QASM. The design of quantum block IR itself is mature to support heterogeneous computing already. And since some companies are interested in using it, heterogeneous shows the ability of Yao that can be extended to their own hardware, and if the Harvard group is going to use it, we (or at least you, you are the guy got hired) need to support their hardware at least.

So what would you suggest here? a link? or?

Copy link
Member

@GiggleLiu GiggleLiu Jun 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

So far, we do not have reliable support to QASM. So let's just remove it?

Copy link
Member Author

@Roger-luo Roger-luo Jun 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

You misunderstand what I meant here. Heterogeneous computing is a feature for quantum block IR (note there is "our design enables" as the condition), it's not about QASM and other features of Yao, we didn't say "our design supports"

The whole point of getting rid of T and MatrixBlock is to make it a better design for heterogeneous computing (or it can be hardware related). This feature means you can extend Yao easily to your desired hardware, which doesn't have to be supported by ourselves.

Without this feature of quantum block IR, you can't extend Yao to QASM/CUDA/etc without dealing with the hardware abstraction (like the precision of blocks).

Even you think heterogeneous computing is a hard word to understand for non-CS background people, we can change the language, but it is a very important feature of quantum block IR. I believe this is something deserve to be mentioned in README.

@GiggleLiu
Copy link
Member

It does not change much, I think?

This package is for physists, this is important. My suggestion is

  • Put some graphs that should be interesting to all, like benchmarks.
  • Using a concrete example to show the advantage of easy to use, Hirachical design, IR directly, rather than speaking it out. If a feature can not be shown as an example, it is not true to user.

@Roger-luo
Copy link
Member Author

This goal of this PR is to make it more native in English. Not to change the contents. I'm working on documentation for benchmarks, will add it in another PR.

I don't think it is good to put examples to README, it is painful to read a huge README. It is better to be short and straight forward. We can provide links to documentation and examples. A glance of the interface (like a 3 line QFT code) might be sufficient, but I don't want to make README huge, that should be in documentation and front page.

@GiggleLiu
Copy link
Member

GiggleLiu commented Jun 6, 2019 •
edited
Loading

its not good to put examples to README

Can't agree, but agree with you that README should not be too long, and current README is already too long. Making it concise does not mean containing no example at all.

I can look into it later.

I am ok to merge this PR.

@Roger-luo
Copy link
Member Author

I mean we can't put all the examples to README, this is not a good README. A glance of code with reasonable size, like an example of QFT is good, since it only has 3 lines. But anything larger should be in documentation or a front page instead of putting it in README.

@GiggleLiu GiggleLiu closed this Jun 6, 2019
@GiggleLiu GiggleLiu reopened this Jun 6, 2019
@Roger-luo
Copy link
Member Author

Roger-luo commented Jun 8, 2019 •
edited
Loading

Let's merge this and revise things after the paper is done? @GiggleLiu

@GiggleLiu
Copy link
Member

ok

@GiggleLiu GiggleLiu merged commit 92d8a8e into master Jun 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GiggleLiu GiggleLiu GiggleLiu left review comments

@wangleiphy wangleiphy wangleiphy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Roger-luo @coveralls @GiggleLiu @wangleiphy