Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

added json editor + fix bug #18

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

added json editor + fix bug #18

wants to merge 5 commits into from

Conversation

liorella-qm
Copy link
Contributor

new features:
this version adds a simple json editor that allows viewing the contents of the DB and editiing them manually. Synchronization between the edited version and the DB version is the responsibility of the user.

fixes:
a bug in update_intermediate_frequency in quaconfig

@liorella-qm liorella-qm requested review from galwiner and qguyk June 28, 2021 03:52
@github-actions
Copy link

github-actions bot commented Jun 28, 2021

Unit Test Results

  1 files    1 suites   5s ⏱️
33 tests 33 ✔️ 0 💤 0 ❌

Results for commit c5b9fc3.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2021

Codecov Report

❗ No coverage uploaded for pull request base (main@710ee79). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #18   +/-   ##
=======================================
  Coverage        ?   77.06%           
=======================================
  Files           ?       10           
  Lines           ?      946           
  Branches        ?        0           
=======================================
  Hits            ?      729           
  Misses          ?      217           
  Partials        ?        0           
Flag Coverage Δ
unittests 77.06% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 710ee79...c5b9fc3. Read the comment docs.

@galwiner
Copy link
Contributor

Hi lior, the json editor implementation is the additional class you added right? I Couldn't see a front end of any kind. Does it interface to somewhere?

@qguyk qguyk requested review from talshani and removed request for qguyk June 28, 2021 07:17
Copy link
Contributor

@talshani talshani left a comment

Choose a reason for hiding this comment

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

There are several things that are unclear here.
Please go over them, and lets discuss...

@@ -132,12 +132,12 @@ def update_intermediate_frequency(
mixer = self.data["elements"][element]["mixInputs"]["mixer"]
lo_freq = self.data["elements"][element]["mixInputs"]["lo_frequency"]
found_entry_index = None
for entry in self.data["mixers"][mixer]:
for i, entry in enumerate(self.data["mixers"][mixer]):
Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes we have a single entry per IF+LO but the data structure doesn't require it.
Also, after we find a result we are still looping

Perhaps change it to a more pythonic way

found_entry_index = next(i for i, entry in enumerate(self.data["mixers"][mixer]) if (
                    entry["intermediate_frequency"] == old_if
                    and entry["lo_frequency"] == lo_freq
                ))

is there a test for the update_intermediate_frequency?

def __init__(self, dbname):
self.dbname = dbname

def write(self, db) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the type of DB?


class QpuEditor(abc.ABC):
@abc.abstractmethod
def write(self, db):
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing type, this interface is not clear

from persistent import Persistent


class CalState(IntEnum):
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving classes and types outside the qpudatabase.py file is not clear.
the name of this file is also unclear and doesn't imply what it should contain

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants