Defining the public API #8

rajibchakravorty · 2019-06-11T04:45:00Z

Major changes

QCtrlObject removed from package
repr is carried out by method create_repr_from_attributes(..) in base.utils module
__all__ introduced in package and sub-package
public methods for creating specific DDS and Driven Control are changed to private methods; only new_predefined_dds or new_predefined_driven_control are public
qiskit package method renamed to covert_dd_to_qiskit_quantum_circuit

…ks; base object removed; repr method created

charmasaur · 2019-06-11T05:50:56Z

qctrlopencontrols/driven_controls/driven_control.py

+            'name': self.name
+        }
+
+        class_name = '{0.__class__.__name__!s}'.format(self)


Rather than requiring each class do this manually, could we change create_repr_from_attributes to take the object itself and do this formatting internally?

I like that too; in that case we can also supply a list of attributes instead of dicts; we then collect both the name and the attribute values from the object (and using the list)

Updated for review

qctrlopencontrols/dynamic_decoupling_sequences/dynamic_decoupling_sequence.py

charmasaur · 2019-06-11T05:55:26Z

qctrlopencontrols/driven_controls/__init__.py

-    new_corpse_in_scrofulous_control)
+from .predefined import new_predefined_driven_control
+
+__all__ = ['UPPER_BOUND_RABI_RATE', 'UPPER_BOUND_DETUNING_RATE',


IMO it'd be best to have one item per line, in alphabetical order. Makes scanning the code a little easier.

Updated; Made the list with one liberty [ALL CONSTANTS, AllClasses, all_methods] within each, alphabetical order in maintained.

charmasaur · 2019-06-11T05:58:17Z

qctrlopencontrols/driven_controls/__init__.py

-    new_corpse_in_scrofulous_control)
+from .predefined import new_predefined_driven_control
+
+__all__ = ['UPPER_BOUND_RABI_RATE', 'UPPER_BOUND_DETUNING_RATE',


I can't remember what we decided for all in subpackages -- did we decide it was necessary, or just that the API of qctrlopencontrols is just the contents of the top-level all?

I cannot remember either; However, though it might be useful to declare all for sub-packages; Can decide otherwise too;

charmasaur · 2019-06-11T05:58:35Z

qctrlopencontrols/__init__.py

-from .cirq import (convert_dds_to_cirq_circuit,
-                   convert_dds_to_cirq_schedule)
+from . import dynamic_decoupling_sequences
+from .dynamic_decoupling_sequences import *


I'm not sure about this -- it means the user has to look into the sub-packages to see the API. Although I suppose they have to do that anyway, to look at the method documentation...

How verbose will it be if we list out all the imports explicitly?

This is related to the other 2 comments [above and below this one]; if we decide all goes only in the top level, this should take care of this problem; Otherwise, I was following numpy case e.g. https://github.com/numpy/numpy/blob/master/numpy/__init__.py; again happy to go with either decision.

OK, happy to be consistent with numpy. If we find it awkward going forwards we can always change it.

charmasaur · 2019-06-11T05:58:57Z

qctrlopencontrols/__init__.py

+from . import cirq
+from .cirq import *
+
+__all__ = []


Similarly, this would be more explicit if we could just list out all the appropriate methods/constants/classes.

# This is the 1st commit message: starting pyquil support # This is the commit message #1: pyquil support added # This is the commit message #2: first version of pyquil notebook added # This is the commit message #3: pyquil dependency added to setup # This is the commit message #4: trying gcc install for pyquil # This is the commit message #5: replacing gcc install with build-essential # This is the commit message #6: forcing yes to install anything # This is the commit message #7: ignoring blocks in pyquil notebook for tests # This is the commit message #8: linted; unnecessary import removed from notebook # This is the commit message #9: removed all references of circuits and replaced those with programs; pyquil removed from method names # This is the commit message #10: notebook rerun after changes # This is the commit message #11: pyquil program conversion removes unnecessary method

# This is the 1st commit message: starting pyquil support # This is the commit message #1: pyquil support added # This is the commit message #2: first version of pyquil notebook added # This is the commit message #3: pyquil dependency added to setup # This is the commit message #4: trying gcc install for pyquil # This is the commit message #5: replacing gcc install with build-essential # This is the commit message #6: forcing yes to install anything # This is the commit message #7: ignoring blocks in pyquil notebook for tests # This is the commit message #8: linted; unnecessary import removed from notebook # This is the commit message #9: removed all references of circuits and replaced those with programs; pyquil removed from method names # This is the commit message #10: notebook rerun after changes # This is the commit message #11: pyquil program conversion removes unnecessary method # This is the commit message #12: Public methods are bundled as __all__; changed ported over to notebooks; base object removed; repr method created # This is the commit message #13: additional error handling for new repr method # This is the commit message #14: repr method updated with class instance as input

…__ of sub-package

rajibchakravorty · 2019-06-13T00:32:32Z

Made the public API (methods and classes) explicit in top-level init
Constants in sub-packages are transferred to sub-packages' own init

charmasaur · 2019-06-13T00:48:20Z

Nice, looks good. The explicit relative paths used internally feel good to me -- nice and clear, and also mean that "qctrlopencontrols.foo" should only appear when foo is part of the public API, and that's true both inside and outside the package.

Rajib Chakravorty added 2 commits June 11, 2019 12:40

Public methods are bundled as __all__; changed ported over to noteboo…

7364518

…ks; base object removed; repr method created

additional error handling for new repr method

be4b5b0

rajibchakravorty requested review from charmasaur and michaelhush June 11, 2019 04:45

charmasaur reviewed Jun 11, 2019

View reviewed changes

repr method updated with class instance as input

75019db

charmasaur approved these changes Jun 11, 2019

View reviewed changes

michaelhush changed the title ~~Improvement~~ Defining the public API Jun 12, 2019

__all__ in sub-packages removed; constants moved to respective __init…

20f32f9

…__ of sub-package

rajibchakravorty force-pushed the tech-improvement branch from 35e39b1 to 20f32f9 Compare June 13, 2019 00:22

michaelhush approved these changes Jun 13, 2019

View reviewed changes

michaelhush merged commit 36d17d8 into master Jun 14, 2019

michaelhush deleted the tech-improvement branch June 14, 2019 05:30

Defining the public API #8

Defining the public API #8

Uh oh!

Conversation

rajibchakravorty commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajibchakravorty commented Jun 13, 2019

Uh oh!

charmasaur commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

charmasaur commented Jun 13, 2019 •

edited

Loading