-
Notifications
You must be signed in to change notification settings - Fork 5
Fix JMAG Naming Issue #345
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
Conversation
- main mach_cad file - tutorial 5 file
|
Thanks @dmnewman3. Does this solution require an earlier version of JMAG (i.e., < v23) to be installed? |
@elsevers This solution does NOT require an earlier version of JMAG to be installed. I found the following if we tell JMAG to use the 21.1 geometry editor:
I did a check with this using Severson02, which only has v21.1 and v23.0 and was able to assign the geometry editor to v22.1 and it functioned properly and imported the regions with the original naming convention. |
|
Thank you for updating @dmnewman3. I only have JMAG v22 and v23 installed on my laptop, but the naming issue still occurred. In my understanding, the reason the material are not set correctly is that the Part name are not set correctly (such as /Region). I found that the groups of Magnet and Coils are not designated as /Region because they were "renamed" upon grouping within eMach. If you ungroup these components in JMAG, the original Part name, including /Region, will become visible. Approach 1: As indicated the email from Anvar, we can use Part ID instead of the Part name when specifying the materials. While we can proceed with this approach, this means that the Part names will remain as odd ones like StatorCore/Region. Approach 2: This approach involves directly changing the Part names. I have tried this approach and pushed to this PR. I made sure the eMach tutorial 5 works at least. By the way, while I was checking the Part ID in eMach, I noticed that the StatorCore ID was assigned incorrectly. When I checked the Part ID List in eMach, it looked like this:
However, in JMAG, the StatorCore ID was I have updated this as well. Unfortunately, this incorrect ID does not seem to be causing the current bug. What do you think about this approach @dmnewman3? If you agree with it, could you run the code and check for any errors? |
Thank you @noguchi-takahiro, it seems both approaches achieve the same goal. I like the second approach you used (and pushed to the branch) since it involves using the names originally assigned. I think it is best we get @elsevers input, since once you and I agree on this, this PR would be ready for his review. I verified the code functions on my end using JMAG23. |
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.
Nice @dmnewman3 and @noguchi-takahiro! Here's a quick review with a couple requests before a more official L2 review.
After making these changes, please test the code with both versions of JMAG and confirm that it works.
mach_eval/analyzers/electromagnetic/bspm/electrical_analysis/JMAG.py
Outdated
Show resolved
Hide resolved
- remove references to version 21.1 - remove extra stator id naming line
|
@elsevers All set, everything still works correctly with the tutorial in question. |
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 for updating @dmnewman3. I also checked It was working in both v22 and v23.
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? N/A
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? N/A
Is PR approved to Level 2? Yes
@elsevers I approve this PR. Could you review this?
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 and @noguchi-takahiro.
I see that this PR is only making changes to the bspm analyzer code. We need to test if other analyzers that use JMAG are impacted. Analyzers to test and fix if broken:
@noguchi-takahiro Can you tackle items 3 and 4 here? I've already done 1 and 2. All that is required is to rename the regions in question. I think you have more experience with these two analyzers than I do. Please push the changes once you've made them and I can re-request a review from @elsevers. |
@elsevers All four of these issues have been addressed:
|
|
@noguchi-takahiro @elsevers Can one of you review this PR so we can get this merged? |
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.
Level 2 review summary: Thanks @dmnewman3. This looks ready to merge to me.
- Does the code comply with the code guidelines? Yes
- Does the code documentation comply with the documentation guidelines? Not applicable
- Is the writing, grammar, and syntax correct and clear? Not applicable
- Is the changeset compliant with the eMach architecture? Yes
- Are the physics accurate? Not applicable
- Did the reviewer change the release schedule for the issues bing closed? Yes, I moved it to a minor revision because it is a bug fix
- Did the reviewer change the branch the PR is being merged into? No, this is correctly configured to merge into
develop - Level 1 re-review instructions (if revisions are requested): No re-review necessary.
- When your PR is approved, remember to merge via
Squash and Merge. Please ask if you are unsure of how to do this.
|
Thanks @elsevers, |








Issue
Address issue #332
Summary
This PR fixes the naming issues present with JMAG 23.0. This solution renames the parts in question affected by the new naming convention.
closes #332