Skip to content

issue45, create 3f_60s and 5f_60s - #49

Merged
sadda merged 44 commits into
mainfrom
45-create-3f-5f-60s
Sep 30, 2026
Merged

sadda merged 44 commits into
mainfrom
45-create-3f-5f-60s

Conversation

@haitao-ZCU

Copy link
Copy Markdown
Collaborator

According to point 1 in issue 45,

  • create synrm_3f_60s.py
  • create synrm_5f_60s.py
  • copy-paste winding.py from vs_5f branch into machine_design/ folder
  • create the test file: test_ansys_synrm_60s_solve.py
  • test passed

@sadda

sadda commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

@haitao-ZCU Is there a reason why we have the following imports?

from machine_design.designs.computation import ComputationBase
from .synrm_3f_36s import Geometry as BaseGeometry

Wouldn't it make more sense to import Computation also from synrm_3f_36s? The same goes for the other machine.

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

@sadda
Agreed. I have simplified these two files and test (test_ansys_synrm_60s_solve) passed.

@sadda

sadda commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU I think that after this change, most methods from Computation should disappear because they have already been inherited. Or am I wrong?

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

@sadda
You are right, but we need to do something first:

  • move unnecessary vars of 3f_36s into python like 5f_40s, with this we can delete set_output_vars() in 3f_60s;
  • modify 3f_36s amd 5f_40s with phase_groups() and winding.py ( in vs_5f branch, machine_design/ ) instead of hard-code coils, with this, we can delete assign_stator_coils() in 3f/5f_60s.

so, if we need to do that, I think I should do that first.

@sadda

sadda commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU I still do not understand why the computation of InitPos is hard-coded in set_oper_params (such as self.InitPos = -30) instead of calling the new function compute_initpos. I imagine a method similar to

def set_initpos(self) -> None:
    g = self.geometry
    Q = int(g.geom_params["SlotNumber"])
    self.InitPos = compute_initpos(Q, g.PolePairs, g.PhaseNumber, g.BeltOffset)
    self.oper_params["InitPos"] = f"{self.InitPos}deg"

Is there a reason for that, or am I missing something?

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author
  • I found a units bug with 5f_40s machine, and have fixed it in this PR (we haven't found it previously maybe the lack of "pythonpath = ["."]" in pyproject.toml ), now the magnitudes of each var is as follows (with Id1=Iq1=7.0711, Id3=Iq3=0),
    V_d1: 116.5
    V_q1: 467.6
    V_d3: 8.876
    V_q3: -26.25
    Flux_d1: 1.737
    Flux_q1: 0.7009
    Flux_d3: 0.03138
    Flux_q3: 0.04317
    Vind_d1: -17.88
    Vind_q1: 333.2
    Vind_d3: 8.876
    Vind_q3: -26.25
    I_d1: 7.071
    I_q1: 7.071
    I_d3: 3.274e-15
    I_q3: -1.637e-15
    Ld1: 0.03716
    Ld1q1: -0.02326
    Lq1: 0.07239
    Ld1d3: 0.0002866
    Ld1q3: -4.647e-05
    Lq1d3: -0.003766
    Lq1q3: -0.004137
    Ld3: 0.01093
    Ld3q3: 0.0002191
    Lq3: 0.01359
    Flux_e_d1: 1.638
    Flux_e_q1: 0.3535
    Flux_e_d3: 0.05598
    Flux_e_q3: 0.07275
    Moving1.Torque: 28.43

  • use set_initpos() function in 3f_36s and 5f_40s, and delete corresponding functions in 3f/5f_60s

  • simplify set_post_params(), previously we use it because Ansys computes the corresponding vars, so we use Ansys plot these vars distribution, but now we have moved unnecessary vars into the python side, so the function set_post_params() should be simplified, and the same function in 3f/5f_60s can be deleted.

  • all tests passed, including Ansys part.

@sadda

sadda commented Sep 29, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU

  • The changes look fine.
  • The tests fail. Please fix them.
  • Please stop chaining multiple issues into one PR. It is very difficult to follow. What is the error which you found? What did it influence? Was it fixed everywhere?
  • Why did you delete the things from set_post_params?

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

@sadda

  • the error seems to be caused by the convergence of InitPos ('InitPos': '-45.00000000000001deg',), I'm trying to fix it.
  • I delete things from set_post_params() because we have moved part of them into python side, for example, we use set_out_vars() to define I_A and use ansys to compute I_A previously, but it has been moved into python side, ansys can only recognize the ansys fields and couldn't recognize I_A, so it's no necessary to put it any more in set_post_params().
  • I agree with that I should split all these changes into different PRs.

@sadda

sadda commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU Ok, please revert the changes about fixing the units and fix the test so that we can close this PR. Then start another PR with the wrong values - Honza will need to check it.

@sadda

sadda commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU It looks ok now. The last thing to suggest would be to move set_initpos into the abstract parent class instead of the anchors. It would need to extract the number of phases.

@haitao-ZCU

Copy link
Copy Markdown
Collaborator Author

ok, I have moved it into the abstract parent class and simplified corresponding part in 3f/5f_60s, now all tests passed except "test_ansys_regression_synrm_5f_40s.py", but when I make some changes in PR #54 , the last test also passed.

@sadda

sadda commented Sep 30, 2026

Copy link
Copy Markdown
Collaborator

@haitao-ZCU

  • When looking at the function which I posted earlier, why is it necessary to define new attributes instead of using g.PhaseNumber and g.BeltOffset?
  • I do not understand the test failing. The tests were ok before this PR. How is it possible that they suddenly failed? Was is something incorporated in this PR? How is fix voltage units #54 related to it?

@sadda
sadda merged commit 0004b2c into main Sep 30, 2026
6 checks passed
@haitao-ZCU
haitao-ZCU deleted the 45-create-3f-5f-60s branch October 1, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants