Developing the complex VNA analysis #492

fluthi · 2018-08-02T13:21:07Z

Please use the following template for a pull request.

Changes proposed in this pull request:

This is to get the MAv2 working for the standard (non-complex) VNA fitting
Changed the timestamp extraction in the options dict of MAv2
Additional wisdom will follow

In order for the pull request to be merged, the following conditions must be met:

travis test suite passes
all reasonable issues raised by codacy must be resolved
a positive review is required

Whenever possible the pull request should

follow the PEP8 style guide
have tests for the code
be well documented and contain comments

Tests are not mandatory as this is generally hard to make for instruments that interact with hardware.

Includes: - Generalization of the base analysis to also run the minimizing method for fitting - Complex resonator fit for the VNA analysis, based on an initial guess using the hanger fitting - Fitting models updated to match the complex fitting

coveralls · 2018-08-14T15:51:57Z

Pull Request Test Coverage Report for Build 3645

126 of 182 (69.23%) changed or added relevant lines in 4 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.3%) to 38.47%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pycqed/analysis_v2/spectroscopy_analysis.py	25	28	89.29%
pycqed/analysis_v2/base_analysis.py	62	80	77.5%
pycqed/analysis/fitting_models.py	24	59	40.68%

Files with Coverage Reduction	New Missed Lines	%
pycqed/analysis_v2/base_analysis.py	1	69.42%

Totals
Change from base Build 3541:	0.3%
Covered Lines:	20274
Relevant Lines:	52701

💛 - Coveralls

AdriaanRol

I have some small comments (see review). Also, please do not commit changes to the hdf5 files that get triggered by the tests. (no blind commit all). After we discuss this we can approve.

AdriaanRol · 2018-08-14T15:53:30Z

pycqed/analysis/fitting_models.py

@@ -310,33 +312,10 @@ def HangerFuncAmplitude(f, f0, Q, Qe, A, theta):
    return abs(A * (1. - Q / Qe * np.exp(1.j * theta) / (1. + 2.j * Q * (f / 1.e9 - f0) / f0)))


-def HangerFuncComplex(f, pars):


I'm assuming this is being deleted on purpose?

The standard complex hanger function should, just like the standard for the fitting models, take floats as arguments.
In order no to loose functionality the function taking a list of parameters as been renamed to hanger_func_complex_SI_pars.
I've added a comment to this.

AdriaanRol · 2018-08-14T15:53:53Z

pycqed/analysis_v2/base_analysis.py

@@ -24,6 +24,8 @@
 import h5py
 from pycqed.measurement.hdf5_data import write_dict_to_hdf5

+from importlib import reload  # Useful for reloading while testing


Please remove this from base analysis.

AdriaanRol · 2018-08-14T15:54:36Z

pycqed/analysis_v2/base_analysis.py

@@ -138,11 +140,14 @@ def __init__(self, t_start: str = None, t_stop: str = None,
        # These options determine what data to extract #
        ################################################
        scan_label = self.options_dict.get('scan_label', label)
+        if scan_label is None:


Would be much better to instead replace the default argument with '', a hidden if statement that replaces some parameter is quite horrible for debugging.

The default argument is indeed a str='', see definition of the init of the base_class.

So in principle the if statement could be removed.

if you agree, could you remove this addition?

Tests fail without it.
Cleanup of this should be addressed in a different pull request. So I'm going to change it back.

Culprit is the default label in the spec analysis. This is now changed, and the if statement removed.
So should be all good now.

AdriaanRol · 2018-08-14T15:55:06Z

pycqed/analysis_v2/base_analysis.py

@@ -224,7 +231,7 @@ def run_analysis(self):
            self.plot(key_list='auto')  # make the plots

        if self.options_dict.get('save_figs', False):
-            self.save_figures(close_figs=self.options_dict.get('close_figs', False))
+            self.save_figures(close_figs=self.options_dict.get('close_figs', True))


good catch 👍

AdriaanRol · 2018-08-14T15:56:29Z

pycqed/analysis_v2/base_analysis.py

@@ -470,6 +477,7 @@ def prepare_fitting(self):
        # initialize everything to an empty dict if not overwritten
        self.fit_dicts = OrderedDict()

+
    def run_fitting(self):


Danger! I think this piece of code is getting awfully complicated to understand (as you have probably experienced when debugging the tests) I am sure the tests are working now but I'm a bit hesitant to approve this change... Let's discuss a bit, I'm not sure what the best solution is.

Added extensive comments that explain what arguments can be passed to the run_fitting function.
Added some comments to clarify what the requirements for the different fitting methods are.
Added extensive comments how the guessing can be done / how guess parameters can be passed on.

AdriaanRol · 2018-08-14T15:57:13Z

pycqed/analysis_v2/spectroscopy_analysis.py

@@ -13,8 +13,11 @@
 from pycqed.analysis import fitting_models as fit_mods
 import lmfit

+from importlib import reload  # Useful for reloading while testing


Please remove debugging stuff from here.

Removed all the reload commands, also to reload the base_class and the fitting_mods

AdriaanRol · 2018-08-14T15:57:27Z

pycqed/analysis_v2/spectroscopy_analysis.py

@@ -13,8 +13,11 @@
 from pycqed.analysis import fitting_models as fit_mods
 import lmfit

+from importlib import reload  # Useful for reloading while testing
+
 import importlib


Especially if it is imported multiple times.

AdriaanRol · 2018-08-14T15:58:11Z

pycqed/analysis_v2/spectroscopy_analysis.py

@@ -210,22 +215,22 @@ def process_data(self):
        self.proc_data_dict['amp_label'] = 'Transmission amplitude (V rms)'
        self.proc_data_dict['phase_label'] = 'Transmission phase (degrees)'
        if len(self.raw_data_dict['timestamps']) == 1:
-            self.proc_data_dict['plot_phase'] = np.unwrap(np.pi / 180. * self.proc_data_dict['plot_phase']) * 180 / np.pi
+            self.proc_data_dict['plot_phase'] = np.unwrap(self.proc_data_dict['plot_phase'],discont=3.141592653589793)


why not np.pi instead of 3.14159....

AdriaanRol · 2018-08-15T08:19:43Z

@fluthi @tstavenga , the test you added is failing because the hdf5 file is not added (these are by default ignored in the gitignore). Come find me and we can fix this

AdriaanRol · 2018-08-16T11:08:39Z

Hi @fluthi you comment that the changes have been made but there are no new commits. Could you push the changes?

fluthi · 2018-08-16T15:09:16Z

Merged and deleted

First working version for standard case

a19b01d

fluthi assigned elrama- and NielsBultink Aug 2, 2018

fluthi requested a review from elrama- August 2, 2018 13:21

fluthi added 3 commits August 4, 2018 16:55

Working complex resonator fitting

dea8248

Includes: - Generalization of the base analysis to also run the minimizing method for fitting - Complex resonator fit for the VNA analysis, based on an initial guess using the hanger fitting - Fitting models updated to match the complex fitting

Upload Before reseting ducati PC

b6693c0

With all other tests working

09e4be1

AdriaanRol suggested changes Aug 14, 2018

View reviewed changes

And the test for it...

091d0da

AdriaanRol mentioned this pull request Aug 15, 2018

Develop awgv1 devices #495

Merged

fluthi added 4 commits August 16, 2018 14:07

Changes to address Adriaans comments

975ddfd

added missing test file

2bb6a7d

the infamous if statement

d1d6dc1

Changing back the if statement

df4e375

AdriaanRol approved these changes Aug 16, 2018

View reviewed changes

And back again :)

c6f3b4a

fluthi merged commit dd7a451 into master Aug 16, 2018

fluthi deleted the VNA_analysis_development branch August 16, 2018 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Developing the complex VNA analysis #492

Developing the complex VNA analysis #492

fluthi commented Aug 2, 2018

coveralls commented Aug 14, 2018 •

edited

Loading

AdriaanRol left a comment

AdriaanRol Aug 14, 2018

fluthi Aug 16, 2018

AdriaanRol Aug 14, 2018

fluthi Aug 16, 2018

AdriaanRol Aug 14, 2018

fluthi Aug 16, 2018 •

edited

Loading

AdriaanRol Aug 16, 2018

fluthi Aug 16, 2018

fluthi Aug 16, 2018

fluthi Aug 16, 2018

AdriaanRol Aug 14, 2018

AdriaanRol Aug 14, 2018

fluthi Aug 16, 2018

AdriaanRol Aug 14, 2018

fluthi Aug 16, 2018

AdriaanRol Aug 14, 2018

AdriaanRol Aug 14, 2018

fluthi Aug 16, 2018

AdriaanRol commented Aug 15, 2018

AdriaanRol commented Aug 16, 2018

fluthi commented Aug 16, 2018

		@@ -310,33 +312,10 @@ def HangerFuncAmplitude(f, f0, Q, Qe, A, theta):
		return abs(A * (1. - Q / Qe * np.exp(1.j * theta) / (1. + 2.j * Q * (f / 1.e9 - f0) / f0)))


		def HangerFuncComplex(f, pars):

Developing the complex VNA analysis #492

Developing the complex VNA analysis #492

Conversation

fluthi commented Aug 2, 2018

coveralls commented Aug 14, 2018 • edited Loading

Pull Request Test Coverage Report for Build 3645

💛 - Coveralls

AdriaanRol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fluthi Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdriaanRol commented Aug 15, 2018

AdriaanRol commented Aug 16, 2018

fluthi commented Aug 16, 2018

coveralls commented Aug 14, 2018 •

edited

Loading

fluthi Aug 16, 2018 •

edited

Loading