zlatko-minev / pyEPR

Powerful, automated analysis and design of quantum microwave chips & devices [Energy-Participation Ratio and more]
https://pyepr-docs.readthedocs.io
Other
160 stars 219 forks source link

analyze_variation incorrectly chooses Pj, Sj, Om, PHI_zpf when passed a subset of modes #148

Closed clarayfontaine closed 1 year ago

clarayfontaine commented 1 year ago

Hello! Thanks so much for providing and maintaining pyEPR, it's easy and versatile to use!

The analyze_variation function can take an argument "modes: list or slice of modes to include in the analysis. None defaults to analysing all modes."

Let's say I have N total modes in my analysis, but I'd like to analyze just M, M < N. I can specify any subset of modes, e.g. [1, 2, 3], [0, 4, 5]

Rather than looking at the values of my modes array, the core_quantum_analysis script always pick the first N values of the PJ, SJ, Om, and PHI_zpf arrays, where these arrays correlate to modes sorted in increasing mode frequency. For example, if I pass in a modes of length 3, regardless of the values of modes, the function will return the parameters corresponding to modes [0, 1, 2]. Instead, it should look at the values I passed in my modes array, and select the PJ, SJ, Om, and PHI_zpf rows that match those values. Otherwise, the calculated Kerr values are incorrect.

See the snippet below from 0.8.5.7:

if modes is not None:

freqs_hfss = freqs_hfss[range(len(self.modes[variation])), ] PJ = PJ[range(len(modes)), :] SJ = SJ[range(len(modes)), :] Om = Om[range(len(modes)), :][:, range(len(modes))] PHI_zpf = PHI_zpf[range(len(modes)), :] PJ_cap = PJ_cap[:, junctions]

I believe the correct snippet, taken from 0.8.4.6, should be something along the lines of:

if modes is not None:

freqs_hfss = freqs_hfss[variation].values[(modes)] PJ = PJ[modes, :] SJ = SJ[modes, :] Om = Om[modes, :][:, modes] PHI_zpf = PHI_zpf[modes, :] PJ_cap = PJ_cap[:, junctions]

Could you clarify and double check? Thanks again :)

github-actions[bot] commented 1 year ago

👏👏👏 You are awesome! Thank you for making your first issue to pyEPR ' first issue

zlatko-minev commented 1 year ago

Thank you very much for finding this bug and for identifying a solution. Indeed the previous code Doesn’t do what it should and will just pick out the first few modes.

It looks like your solution should work. Have you given it a quick test?

I would suggest that you make a poll request with this changes. Since it’s only a few lines, you could even do it in the online web interface by editing the file directly and clicking make pull request which I can then review and approve

nikosavola commented 1 year ago

I think this issue should be closed as of 148edc0

zlatko-minev commented 1 year ago

Thanks Niko