-
Notifications
You must be signed in to change notification settings - Fork 5
Restructure Flux Linkage and Inductance Analyzers #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Restructure Flux Linkage and Inductance Analyzers #343
Conversation
…lyzer # Conflicts: # docs/source/EM_analyzers/inductance_analyzer.rst # examples/mach_eval_examples/SynR_eval/inductance_step.py # mach_eval/analyzers/electromagnetic/flux_linkage_analyzer_config.py
wchan29
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmnewman3 Here is my Level 1 review summary:
Does the code run without error and produce the expected result? Yes
Does the code comply with the code guidelines? Yes
Does the code documentation comply with the documentation guidelines? Yes
Is the writing, grammar, and syntax correct and clear? Yes
Is the changeset compliant with the eMach architecture? Yes
Does this review consider whether this physics are accurate? Not sure, but seems good.
Is PR approved to Level 2? Yes
|
@dmnewman3 -- thanks! I have asked @noguchi-takahiro to do a review on this. Once you get an approval from him, I'll go ahead and do a review as well. |
- main mach_cad file - tutorial 5 file
noguchi-takahiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @dmnewman3. Your flux and inductance analyzer works well. I have super small request regarding the printing the rotor_angle. Please see the comment below and consider to update it.
examples/mach_eval_examples/SynR_eval/SynR_flux_linkage_step.py
Outdated
Show resolved
Hide resolved
noguchi-takahiro
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @dmnewman3. It looks good to me.
Here is my Level 1 review summary:
Does the code run without error and produce the expected result? Yes
Does the code comply with the code guidelines? Yes
Does the code documentation comply with the documentation guidelines? Yes
Is the writing, grammar, and syntax correct and clear? Yes
Is the changeset compliant with the eMach architecture? Yes
Does this review consider whether this physics are accurate? Yes
Is PR approved to Level 2? Yes
@elsevers I approve this PR. Could you review this?
- remove references to version 21.1 - remove extra stator id naming line
elsevers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review here. I am comparing the plan in issue #296 to the documentation I am reading for the flux linkage analyzer and think we aren't quite on the right track.
- Flux linkage analyzer input 1: instead of taking in a SynR machine object, instead take in a JMAG tool handle to a JMAG project that is open and already loaded with the machine and fully configured. It could be a SynR machine, or a PM machine, or something else entirely -- doesn't matter.
- Flux linkage analyzer input 2: total number of phases
- Flux linkage analyzer input 3: name of each phase
- Flux linkage analyzer input 4: list consisting of objects of our standard operating point class to calculate flux linkages at.
The analyzer then calculates the flux linkages for all phases at each operating point.
One thing I am not sure of: does our operating point class give us capability to specify each phase current individually?
Takahiro and I will discuss a bit more later today and revise this description.
|
Hi @noguchi-takahiro, I've updated the analyzers and incorporated @elsevers feedback. I think I've got it to a point where I can have you look over it. It have it to a point where everything is structured as Eric requested and it is fully functional with the example file I pushed to the repo. I've also updated the documentation quite a bit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change requests for flux linkage analyzer and problem
- problem: rename to
Flux_Linkage_ProblemtoFluxLinkageJMAG_Problem - problem: Use the
toolJmag = JMAG.JmagDesigner()instead ofappandmodel - problem + analyzer: let's eliminate using CSV files to exchange information. Instead, we'll save flux linkage data to an object. In the problem class, see if you can prevent JMAG from storing the data onto the hard drive. Just retrieve it directly from JMAG via a function call.
- problem:
ratedcurrentshould be renamed torated_current - analyzer: should not be referring to JMAG specific information from the problem class. I.e., remove the analyzer using app or model.
- problem: remove the requirement that a user needs to make the mesh prior to creating the problem. Our theory is that creating the mesh as a distinct step is unnecessary and that just calling the
runfunction will take care of this. The model should be pre-configured with the appropriate mesh settings prior to creating the problem object.
|
Thanks for the feedback @elsevers and @noguchi-takahiro, I have made the changes requested:
Please re-review and provide feedback as necessary. |
elsevers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmnewman3, quick review on this caught a problem -- can you take a look?
…lyzer and move them to the problem class
elsevers
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dmnewman3. See my comment on revisions needed to implement the approach we outline in #296
- requirements of model in .rst file - flux_linkage analyzer - inductance analyzer - example JMAG file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dmnewman3. I left you some items to work on as in-line annotations.
I think we should be including an abstract base class for that the flux linkage analyzer expects of the flux linkage problem class (abstract functions of delete_operating_points, new_operating_point, set_phase_current, and run_analyses) and then have your problem class implement that abstract base class. This approach is used extensively in other parts of the codebase. If you are familiar with how to do this, go ahead and try. Otherwise, please work on addressing the more concrete items I have identified.
examples/mach_eval_examples/fluxlinkage_inductance_eval/fluxlinkage_inductance_evaluator.py
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,103 @@ | |||
| import os | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @elsevers,
I have never seen this issue, but I have implemented a save command right after opening so hopefully that fixes your problem. I have created the file using JMAG21 so there should be no problem opening it in a more recent version. Please do let me know if this most-recent push fixes this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to @noguchi-takahiro and @dmnewman3, this appears to be a bug with JMAGv22. This error did not pop up when using JMAGv21 or JMAGv23.
- remove hard-coded references - save file before executing - redefine add_em_study and remove it as analyzer call - rename and restructure del_op_pts, new_op_pt, run_all_cases - add set_phase_currents - set output as vector of phase currents - update documentation
|
@elsevers I have implemented your feedback, but I don't understand how what we're doing now is not implementing the abstract base classes you are referring to. The pattern we are doing seem to be exactly how is laid out in the guidelines and how other analyzers are structured. I would like to prioritize getting this over the finish line and then if time allows we can improve upon it from there. Either that or maybe I have the wrong idea of what you're referring to. Either way, I am ready for a new review. |

Issue
Closes #296
Summary
This PR does the following:
closes #296