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

Compatibility with Ansys 2022 R2 #142

Closed hzw770 closed 10 months ago

hzw770 commented 1 year ago

When running pyepr on ansys 2022 R2, ansys complained that it does not recognize "re(Mode(1)):Curve1". This could be related to the new version's names of the figure properties.

The way I temporarily fix this is to remove ":Curve1", and also comment out several "set_property" commands in core_distributed_analysis.py .

zlatko-minev commented 1 year ago

Thank you will need to look at this more carefully

Is it backward compatible?

hzw770 commented 1 year ago

I don't currently have old ansys versions installed unfortunately. But it shouldn't matter if one just comment out line 1579 to 1587 in core_distributed_analysis.py because they just edit the figure properties?

zlatko-minev commented 1 year ago

For any of the backward breaking changes you can add an if statement as shown in the ansys.py file that checks the version of ansys and one one thing if old and another if new

Do you know what I mean?

hzw770 commented 1 year ago

I see, let me try

hzw770 commented 1 year ago

For backward compatibility: In core_distributed_analysis.py:

First, import Dispatch at the beginning of this .py from win32com.client import Dispatch

Then replace previous 1579 - 1587, by:

        _app = Dispatch('AnsoftHfss.HfssScriptInterface')
        desktop = _app.GetAppDesktop()
        _ansys_version_ = desktop.GetVersion()
        if _ansys_version_ <= '2022':
            # Properties of lines
            curves = [f"{report_name}:re(Mode({i})):Curve1" for i in range(
                1, 1+self.n_modes)]
            set_property(report, 'Attributes', curves, 'Line Width', 3)
            set_property(report, 'Scaling',
                         f"{report_name}:AxisY1", 'Auto Units', False)
            set_property(report, 'Scaling', f"{report_name}:AxisY1", 'Units', 'g')
            set_property(report, 'Legend',
                         f"{report_name}:Legend", 'Show Solution Name', False)
zlatko-minev commented 1 year ago

Is the code backwards compatible now?

zlatko-minev commented 1 year ago

Wondering if it would be safe to merge this into main, what do you think?

landamax commented 1 year ago

Hi, only wanted to know whether this issue will be merged to main branch soon? I really want to use an original version and not to work with some quick-fixes as comment outs and such. Also it looks like this issue breaks down the EPR analysis in Qiskit Metal.

zlatko-minev commented 1 year ago

You mean this PR break with Qiskit Metal use. Yes, that means it is not compatible in current form. We would want some help to fix and make not quick issues. I also have some questions in the code here too.

zlatko-minev commented 1 year ago

I was also a bit confused before. I now notice that this PR is pulling from main into develop (not into main)

landamax commented 1 year ago

Oh I didn't even notice that it pulls into develop! I noticed this issue became more inclusive: it has some fixes for calculation of q factors, specifically the surface q factor (instead of raising a NotImplementedError it actually performs the calculation, which was implemented about a year ago). Can this issue be pulled into main?

zlatko-minev commented 1 year ago

I tried to change it on my end but got:

image

Can you try on your end? Or you can close this one and reopen it with master

landamax commented 1 year ago

Oh I think I don't have permissions to do something like that, but I see this: to_pyepr_github

so maybe you can merge the develop branch with master after reviewing this issue.

zlatko-minev commented 10 months ago

Thank you for the effort on this massive update! I just merged! Hopefully there aren't any major compatibility issues, i didn;t catch any here