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

improve docstrings #957

Closed
3 tasks done
Roger-luo opened this issue May 23, 2024 · 30 comments · Fixed by #970
Closed
3 tasks done

improve docstrings #957

Roger-luo opened this issue May 23, 2024 · 30 comments · Fixed by #970
Labels
documentation Improvements or additions to documentation unitaryhack

Comments

@Roger-luo
Copy link
Member

Roger-luo commented May 23, 2024

see format in #956

bounty: 20 USD/per 5 docstring

please specify which method/class you will be working on, and we will open a new issue for you

@Roger-luo Roger-luo added documentation Improvements or additions to documentation unitaryhack labels May 23, 2024
@shubhusion
Copy link
Contributor

shubhusion commented May 24, 2024

@Roger-luo Kindly assign this issue to me. I will start with #946

@Roger-luo
Copy link
Member Author

Roger-luo commented May 24, 2024

@shubhusion could you point out the methods and classes you would like to work on (perhaps in the end) then I'll open an issue for you?

@shubhusion
Copy link
Contributor

shubhusion commented May 24, 2024

I intend to do the whole issue since there is a bounty associated with it. I will start with parse folder (https://github.com/QuEraComputing/bloqade-python/tree/main/src/bloqade/builder/parse) all 4 files for now you can consider. ( 9 classes overall)

@Roger-luo
Copy link
Member Author

Ok, that is amazing @shubhusion! just in case of confusion, I updated the bounty description a bit - it should be 20 USD for every 5 docstrings. As per the amount listed on the website, 50 docstrings will be 200 USD.

@shubhusion
Copy link
Contributor

Can I use this issue only or have you created a new issue for improving docstrings ?

shubhusion added a commit to shubhusion/bloqade-python that referenced this issue May 25, 2024
@shubhusion
Copy link
Contributor

Hey @Roger-luo, I've submitted a PR addressing the enhancement of one-liner docstrings. Please review it and let me know if it aligns with your expectations. If there are any discrepancies or further adjustments needed, feel free to point them out. Your feedback will help me understand the project's standards better, guiding my future PRs.

@AbdullahKazi500
Copy link

AbdullahKazi500 commented May 29, 2024

why was the bounty assigned when the hackathon was to start from May 29 @Roger-luo
now you have to close this PR I guess since it is against the rules

@AbdullahKazi500
Copy link

@shubhusion this PR Can be merged but without a bounty since you have broke the Rules

@shubhusion
Copy link
Contributor

@shubhusion this PR Can be merged but without a bounty since you have broke the Rules

@AbdullahKazi500 How did I break a rule? The issue had bounty tag and @Roger-luo accepted that. You can check our discussion.

@AbdullahKazi500
Copy link

The hackathon was to start on May 29 that is today and all hacking and PRs were to made from Today
why was the PR Made 5 days back
@shubhusion

@shubhusion
Copy link
Contributor

The hackathon was to start on May 29 that is today and all hacking and PRs were to made from Today
why was the PR Made 5 days back
@shubhusion

I didn't know that it was part of a hackathon. I assumed it as an open bounty.

@AbdullahKazi500
Copy link

@shubhusion the Bounty tag was to identify that this is a bounty issue not for you to start working on it
since the official hackathons starts today this is breaking the rules
you can merge the PR Without a bounty

@AbdullahKazi500
Copy link

@shubhusion it is not an open bounty and if you have assumed then it is your mistake I guess

@shubhusion
Copy link
Contributor

@shubhusion it is not an open bounty, and if you have assumed, then it is your mistake, I guess

No issues , i will make more PRs for bounties, which will be accepted from today.

Just merge this PR if possible.

@shubhusion
Copy link
Contributor

@AbdullahKazi500 if i make another PR to improve docstrings will that be accepted ?

@AbdullahKazi500
Copy link

@shubhusion nope that PR won't be accepted since you have already made a PR if the new PR is different than the old PR maybe yes
because there is a possibility that you are copying the same code from the Old pr and making it new which is also against the rules
it is better to close this issue without a bounty for now

@shubhusion
Copy link
Contributor

@shubhusion nope that PR won't be accepted since you have already made a PR if the new PR is different than the old PR maybe yes
because there is a possibility that you are copying the same code from the Old pr and making it new which is also against the rules
it is better to close this issue without a bounty for now

Yeah, new PR will be different since this issue involves improving docstring . In this PR, only some docstrings were improved as a starting point. Each PR will involve different files documentation update.

@AbdullahKazi500
Copy link

Then maybe yes you can make a New PR with a different code and that might be okay

@shubhusion
Copy link
Contributor

Then maybe yes you can make a New PR with a different code and that might be okay

@AbdullahKazi500 can you please confirm this?

@Roger-luo
Copy link
Member Author

I got a reply from Kallie (organizer of unitaryhack):

he event started today, so any PRs submitted before today and/or accepted are not eligible for the bounty.

please feel free to submit a new PR if any of you want to work on this (just don't work on the same docstring). You can share the bounty of this issue (I'll open a new issue for you, just list the methods you want to claim)

@shubhusion
Copy link
Contributor

@Roger-luo Kindly assign the following classes to me present in (https://github.com/QuEraComputing/bloqade-python/tree/main/src/bloqade/compiler/analysis/common).
AssignmentScan , CheckSlices , IsConstant , IsHyperfineSequence , ScanChannels , ScanVariables

Roger-luo added a commit that referenced this issue May 30, 2024
* improve one-liner docstring
Fixes #946

* #957 Improve docstrings

* improve one-liner docstring
Fixes #946

---------

Co-authored-by: Xiu-zhe (Roger) Luo <rogerluo.rl18@gmail.com>
@shubhusion
Copy link
Contributor

shubhusion commented May 30, 2024

@Roger-luo many methods do not have docstrings are those also covered in this issue? you told me 50 docstrings for 200$ bounty

@Roger-luo
Copy link
Member Author

Roger-luo commented May 30, 2024

In general, we would prefer adding docstring for the methods the user will touch, e.g., the APIs for task https://github.com/QuEraComputing/bloqade-python/blob/main/src/bloqade/task/base.py#L50

For more specific docstrings to work on, please feel free to comment on the ones you want to work on here for discussion, and @johnzl-777 will also open some issues during the hackathon as guidance. (He will reply in this thread shortly)

Actually I think the task and submissions module has no docstring, and those objects appear in user space so you can pick methods to work on in these two modules.

@Manvi-Agrawal
Copy link
Contributor

Hi @Roger-luo , I created #970 for docstrings in "tasks/base.py" for unitary hack.

@Manvi-Agrawal
Copy link
Contributor

Hi @Roger-luo , I fixed doc strings for some files, could you please take a look?

@Manvi-Agrawal
Copy link
Contributor

Manvi-Agrawal commented Jun 3, 2024

@Roger-luo , give me some time to create separate issues, created one for PR merged recently.
#983
#984

@Roger-luo
Copy link
Member Author

Roger-luo commented Jun 3, 2024

@Manvi-Agrawal Please do not create separate issues, I will count your total amount of bounty and create issues for you because we need to notify on unitaryhack side too.

Regarding who wants to work on the docstring, it might not be clear, as mentioned above, that we don't want a bounty on helper functions that only developers will use or private ones. I made the exception to accept #978 because we didn't say this explicitly.

However, we will not accept docstrings added to helper functions or private methods in the future. Please work on public APIs in the user space, as I mentioned in #957. You should at least look at the quick start and tutorial to understand what this package does and create docstrings for those in the user space (meaning users may use them).

And please DO NOT submit multiple PRs when you still have PR under review, this will not give you more bounties, and it will create a lot of duplicated work for us to review the PR and for you to correct them.

@Manvi-Agrawal
Copy link
Contributor

@Manvi-Agrawal Please do not create separate issues, I will count your total amount of bounty and create issues for you because we need to notify on unitaryhack side too.

@Roger-luo , thanks for your review. I didnt know that this was how it was supposed to be done. I thought of saving you some admin hassle, thats why created the issues.

And please DO NOT submit multiple PRs when you still have PR under review, this will not give you more bounties, and it will create a lot of duplicated work for us to review the PR and for you to correct them.

Sure thing, since it was a documentation change, I thought I would be able to get them in without much hassles :-). I am happy to wait until my other PRs are reviewed.

@shubhusion
Copy link
Contributor

@Roger-luo @AbdullahKazi500 My two PRs were merged successfully. But My Name is not present on the leaderboard. can you guide me whom to contact ?

@Roger-luo
Copy link
Member Author

Hi all I'm opening new issues and will contact unitaryhack to track things correctly today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment