Skip to content

Conversation

augustehirth
Copy link
Contributor

@augustehirth augustehirth commented Dec 22, 2021

PR 1 of 6 on implementing and graphing Figures 2d through 3d of Observation of Time-Crystalline Eigenstate Order on a Quantum Processor (arxiv:2107.13571)

This PR implements the Experiment Class used in experiments, defining the parameters to a single instance of the problem (in the DTCTask case), and the function to compare multiple instances of the problem (comparison_experiments)

@augustehirth augustehirth changed the title Add Tasks for Discrete Time Crystal Experiments Add Experiment Class for Discrete Time Crystal Experiments Jan 14, 2022
@augustehirth
Copy link
Contributor Author

I've updated everything to do away with the Tasks paradigm and dataclasses entirely. Let me know if this is a worthwhile change.

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

some nits

@mpharrigan mpharrigan added the area/time_crystals Concerns the time crystals module label Jan 24, 2022
@augustehirth
Copy link
Contributor Author

@XiaoMiQC This is the first DTC PR, ready for review.

Comment on lines 67 to 68
for name in args:
kwargs[name] = None if name is arg else locals()[name]
Copy link
Collaborator

Choose a reason for hiding this comment

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

please don't be fancy like this with tests. Keep them "DAMP" (descriptive and meaningful phrases) rather than "DRY" (don't repeat yourself)

In addition to being clunky, this test is broken when I run it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests should be working properly now, and the code is less fancy.
What they do, however, hasn't changed. They still simply run every combination of default or supplied values for path coverage. This could be improved but I'm not sure how would be best.

Copy link
Collaborator

@mpharrigan mpharrigan left a comment

Choose a reason for hiding this comment

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

looks good from my side. Xiao to sign off re: domain knowledge

@mpharrigan
Copy link
Collaborator

cc @XiaoMiQC

@mpharrigan
Copy link
Collaborator

what's the status of this?

Copy link
Contributor

@XiaoMiQC XiaoMiQC left a comment

Choose a reason for hiding this comment

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

LGTM

@mpharrigan mpharrigan merged commit dea4efe into quantumlib:master Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/time_crystals Concerns the time crystals module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants