Semicon merge #131
Semicon merge #131
Conversation
…ts bugs mercilessly!
…the according times when the jumps happened.
…f overwriting them.
…pin/semispin/qkit into script_based_analysis
…now properly builds x-axis if step is a number which is not an int
…r exceeding max output range.
|
Some notes:
|
|
It looks like a lot of effort went into these changes. Good work! But I will leave some further comments. |
thilokru
left a comment
thilokru
left a comment
There was a problem hiding this comment.
Overall cool, but some major changes are required before this should be merged, in my opinion.
There was a problem hiding this comment.
Shouldn't this in principle be covered by
qkit/src/qkit/core/instrument_base.py
Line 448 in 133f2f0
| if type(Pathobj) is not dict or 'authentication' not in Pathobj: | ||
| raise ValueError("Could not find authentication data in you path object. Please make sure your pathobject is a dictionary containing an 'authentication' key.") | ||
|
|
||
| host = "os-login.lsdf.kit.edu" #hard-coded |
There was a problem hiding this comment.
Hard coded hosts are not a good idea. Also, I think this should be handled on a file system level and not within qkit.
| "IVgain" : 1e8, | ||
| "in_line_R": 42e3, | ||
| "sampling_rate" : 13732.91015625} | ||
| } |
There was a problem hiding this comment.
This does not seem to fit into qkit source, but should rather be some configuration from the notebook. Note, that in the new installation paradigm of qkit, all files in src/qkit should not be user maintainable.
I recommend moving this either to a configuration option, or make this an argument passed to some function.
There was a problem hiding this comment.
I think this file would be better in the documentation folder as an example notebook.
There was a problem hiding this comment.
This configuration method is deprecated.
|
|
||
| import numpy as np | ||
| import json | ||
| from matplotlib.widgets import Cursor |
There was a problem hiding this comment.
Why do you need widgets? Just curious.
There was a problem hiding this comment.
Cool that the unified measurement_base gets used. 👍
| @author: hannes.rotzinger@kit.edu 2018 | ||
| @author: marco.pfirrmann@kit.edu 2018 | ||
| @version: 0.1 | ||
| *modified* |
| """ | ||
| ds = hdf_dataset(self.hf, name, x=x, unit=unit, ds_type = ds_types['vector'], | ||
| comment=comment, folder=folder, dim = 1, **meta) | ||
| comment=comment, folder=folder, dtype='float64', dim = 1, **meta) |
There was a problem hiding this comment.
is dtype=float64 always a sane choice?
There was a problem hiding this comment.
Should also be an example notebook, or a template notebook.
|
I think all of your |
|
We also discussed if the ADWin binary files should be included in qkit. On one hand they're absolutely necessary to run the ADWin box, on the other cryptic files should not be part of a public repo. What I did for the "ADWin as SMU" driver some while ago was putting them on the LSDF and link to them in the docstring. Since working with ADWin OOTB is not that simple for a new user anyways, downloading them is just another step in a longer list of TODOs. What I'd propose is we keep the non-compiled FPGA drivers in the extra folder structure as is and add a README with links to either the LSDF or a separate git repo [EDIT: with the compiled binaries]. We should also see if my driver can be refactored to be included in your more extensive environment [EDIT: or if it has become redundant and can be removed]. |
9 participants
No description provided.