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

[QILI-46] Implement instrument classes / feat #9

Merged
merged 156 commits into from
Apr 19, 2022
Merged

[QILI-46] Implement instrument classes / feat #9

merged 156 commits into from
Apr 19, 2022

Conversation

amitjansc
Copy link
Contributor

@amitjansc amitjansc commented Apr 6, 2022

Added instruments/ folder, which contains:

  • Instrument abstract class.
  • QbloxPulsar / QbloxPulsarQCM / QbloxPulsarQRM / SGS100A classes.

Added inside the settings/ folder:

  • InstrumentSettings, QbloxPulsarSettings, QbloxPulsarQCMSettings, QbloxPulsarQRMSettings, SGS100ASettings and PulseSettings classes.
  • Added these new classes in the SettingsHashTable and SettingsManager classes.
  • Remove the "_settings" from the filenames.

I also removed some __init__ imports and I made sure to never use relative imports in src files.

Note: The instruments classes contain all the necessary methods to connect, setup, upload, run and acquire a sequence. In comparison with the classes of qibolab, these classes do not contain the methods used to:

  • Translate pulses into waveforms and into a program.
  • Integrate acquisition.

Subnote: Since I created this branch from the platform branch, all the commits from the aforementioned branch are listed below... 😩

Subsubnote: I had to delete src from the interrogate args inside the .pre-commit-config.yaml file to avoid checking unstaged files.

amitjansc and others added 30 commits March 23, 2022 14:52
Add qibo and pyyaml requirements
SettingsLoader to SettingsManager
@iamtxena
Copy link
Contributor

  1. I called the generic classes QubitControl and QubitReadout. I followed ZurichInstruments nomenclature. I like this better than PulsarControl and PulsarReadout because it is more generic. Let me know what you think.

Great. Like it.

  1. I added the QbloxPulsar class because, in this specific case, both QRM and QCM share a LOT of the same methods. However, I have no idea if other instruments/pulsars will have the same characteristics, thus I don't know if we should add a generic Pulsar class. This way we follow more closely the schema that you designed.

I would say that we figure this out, when we integrate with either flux qubit simulator or another device (whatever comes first)

  1. The generic classes are completely empty right now. Since all methods of the Qblox classes are Qblox-specific, I couldn't find anything that should go in the generic instruments. For example, I have no idea if ZurichInstruments also need an upload method to upload the program into the instrument. Maybe I should look more into this.

Don't do this for now. Just we have to think that, whenever we need it, then we can refactor.

Just look at SettingsHashTable class. A platform should only know about the generic names of an instrument, but not directly the qblox and rohde_schwartz that is defined there. Again, think of this as a generic one that should admit several platform layouts.

I added generic settings in commit c8f6353. However, this was trickier than I thought. There will always be instrument-specific settings (for example QbloxQRM will need different types of settings than ZurichInstrumentsQRM). If we need to create a dataclass for each setting, then some "object" will sooner or later need to fetch this specific dataclass. How should we do it? Should we add different layers of hashtables...?

Also, how can we be able to fetch the correct settings if the platform doesn't know about the specific instrument that is being used (Qblox, ZurichInstruments...)? I believe in the platform the user should definitely specify the exact instrument that they are using, I can't think of a better solution... 😞

OK, now I get it what you meant about the settings thing to ask it in the StackOverflow. Let's discuss it today, but as I was saying this will be restructured after integrating with qiboconnection.
In general, think that way when dealing with similar issues:

  • You usually need to define a structure that is generic and easy to be extendable for other uses / requirements
  • And you need to define each specific feature into different specific classes
  • Then, and this is the important part, when you instantiate your procedure, you do it calling the specific classes (not the generic), because you as the user, knows exactly what you want.

Copy link
Contributor

@iamtxena iamtxena left a comment

Choose a reason for hiding this comment

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

Some comments, but mostly done.

However, now that we know how to create mocks, I would consider splitting the tests into unit and integration , leave the current tests as integration, and create unit tests with mocked.

.pre-commit-config.yaml Show resolved Hide resolved
src/qililab/backend.py Outdated Show resolved Hide resolved
src/qililab/instruments/qblox/qblox_pulsar.py Outdated Show resolved Hide resolved
src/qililab/instruments/qblox/qblox_pulsar.py Outdated Show resolved Hide resolved
src/qililab/instruments/rohde_schwarz/sgs100a.py Outdated Show resolved Hide resolved
src/qililab/instruments/rohde_schwarz/sgs100a.py Outdated Show resolved Hide resolved
tests/test_backend.py Outdated Show resolved Hide resolved
Copy link
Contributor

@iamtxena iamtxena left a comment

Choose a reason for hiding this comment

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

Super great job and with tests!!!

src/qililab/settings/instruments/qblox/qblox_pulsar.py Outdated Show resolved Hide resolved
@amitjansc amitjansc merged commit 66f5aae into main Apr 19, 2022
@amitjansc amitjansc deleted the instruments branch April 20, 2022 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants