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

[Unitary Hack] Improve docstrings in submission/ir/task_specification.py #982

Closed
wants to merge 2 commits into from

Conversation

shubhusion
Copy link
Contributor

Fixes #957

Added Module , function docstrings and removed Markdown errors.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.91%. Comparing base (9fdc8cb) to head (cbe324f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
+ Coverage   89.90%   89.91%   +0.01%     
==========================================
  Files         110      110              
  Lines        8132     8132              
==========================================
+ Hits         7311     7312       +1     
+ Misses        821      820       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Roger-luo
Copy link
Member

While I do appreciate the hard work you did in your previous PRs, please do not open new PRs if you still have other PRs under review. This is unfair to others who want to work on the same module. And this results in duplicated review comments. I have commented on other PRs; you should have one example demonstrating how to use the API in these user-facing APIs.

@shubhusion
Copy link
Contributor Author

shubhusion commented Jun 3, 2024

While I do appreciate the hard work you did in your previous PRs, please do not open new PRs if you still have other PRs under review. This is unfair to others who want to work on the same module. And this results in duplicated review comments. I have commented on other PRs; you should have one example demonstrating how to use the API in these user-facing APIs.

@Roger-luo I understand what you meant. I am just trying my best to win some more bounties despite being new to the quantum field.

I won't mention anyone now first.

@Roger-luo
Copy link
Member

Roger-luo commented Jun 4, 2024

I am just trying my best to win some more bounties despite being new to the quantum field.

We are happy to provide anything you need to learn about neutral atom computing and quantum information. This is exactly why this hackathon is for, and we understand the hardness. Thus, improving docstring is designed to be a good and easy starting task for learning and joining the field. But submitting more PRs with similar issues won't win you more bounties except for producing more work for everyone.

You really made a lot of good changes, and there are some promising PRs, so I really hope you can finish them and understand what these functions are for and be able to make examples for them.

Copy link
Member

@weinbe58 weinbe58 left a comment

Choose a reason for hiding this comment

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

I think as an addition to the doc strings and examples if you could rename all task_capabilities to device_capabilities I think this PR would be good to go.

The meaning of descretize is to make sore the resulting IR object is compatible with a device so making this change to the argument will communicate this better.

Discretizes the global field values based on the task capabilities.

Args:
task_capabilities (QuEraCapabilities): Capabilities of the task for discretization.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
task_capabilities (QuEraCapabilities): Capabilities of the task for discretization.
task_capabilities (QuEraCapabilities): Capabilities of the target device

Sorry about this, the name of the argument isn't really intuitive here so this isn't really your fault but for context the capabilities for a particular quantum device not a task. This function is used to transform the IR nodes to be more compatible with a particular device.

@@ -51,6 +68,15 @@ def __hash__(self):
return hash((RabiFrequencyAmplitude, self.global_))

def discretize(self, task_capabilities: QuEraCapabilities):
Copy link
Member

Choose a reason for hiding this comment

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

please go ahead and change all argument names task_capabilities to device_capabilities which is more accurate name for what this argument means.

Copy link
Member

@weinbe58 weinbe58 left a comment

Choose a reason for hiding this comment

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

Sorry about the delayed response. These doc strings still needs some work, the examples do not really reflect the intended usage and some of them do not even work when you run them.

Comment on lines +422 to +428
# Create a Detuning
detuning = Detuning(global_=global_field)

# Generate and display the figure
detuning_figure = detuning.figure()
show(detuning_figure)
```
Copy link
Member

Choose a reason for hiding this comment

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

missing Example here?

Comment on lines +246 to +257
# Define example capabilities
class RydbergCapabilities:
class GlobalCapabilities:
time_resolution = Decimal('0.5')
phase_resolution = Decimal('0.1')

global_ = GlobalCapabilities()

class Capabilities:
rydberg = RydbergCapabilities()

device_capabilities = QuEraCapabilities(capabilities=Capabilities())
Copy link
Member

Choose a reason for hiding this comment

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

This is not the intended useage of this function. I suggest using the default capabilities instead of creating your own for the examples. if you want to modify the capabilities, modify the capabilities object directly before passing it into the method.

I would also make sure your examples run, this one is incomplete and doesn't run even if you have the correct imports.

@weinbe58 weinbe58 closed this Aug 2, 2024
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.

improve docstrings
3 participants