QuEraComputing / bloqade-python

QuEra's Neutral Atom SDK for Analog QPUs
https://bloqade.quera.com/
Other
48 stars 12 forks source link

improve docstrings #957

Open Roger-luo opened 1 month ago

Roger-luo commented 1 month ago

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

shubhusion commented 1 month ago

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

Roger-luo commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

shubhusion commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

shubhusion commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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

shubhusion commented 1 month ago

@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 commented 1 month ago

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

AbdullahKazi500 commented 1 month ago

@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 commented 1 month ago

@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 commented 1 month ago

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

shubhusion commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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

shubhusion commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

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

Manvi-Agrawal commented 1 month ago

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

Manvi-Agrawal commented 1 month ago

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

983

984

Roger-luo commented 1 month ago

@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 https://github.com/QuEraComputing/bloqade-python/issues/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 commented 1 month ago

@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 commented 3 weeks ago

@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 commented 3 weeks ago

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