Skip to content

Conversation

@wchan29
Copy link
Contributor

@wchan29 wchan29 commented Jan 10, 2024

Issue

Address issue #325

Summary

This PR adds BSPM Machine Constants Analyzer to eMach codebase.

closes #325

@wchan29 wchan29 marked this pull request as ready for review January 12, 2024 21:17
@wchan29 wchan29 requested a review from dmnewman3 January 12, 2024 21:17
@wchan29
Copy link
Contributor Author

wchan29 commented Jan 12, 2024

@dmnewman3 this is ready for level 1 review

@elsevers elsevers added this to the Jan 24, 2024 Meeting milestone Jan 14, 2024
@wchan29 wchan29 requested a review from npetersen2 as a code owner January 17, 2024 01:17
@dmnewman3
Copy link
Contributor

@wchan29 I merged with the current develop branch and made some alterations to the .rst file. Can you run through the .rst file once more to make sure the changes I made don't change what you intended to write? Other than that things look good.

@wchan29
Copy link
Contributor Author

wchan29 commented Jan 25, 2024

@dmnewman3 I checked the rst looks good.
Ready for 2nd level review. @elsevers @npetersen2 @Anvar-Khamitov

@wchan29
Copy link
Contributor Author

wchan29 commented May 16, 2024

@elsevers

@elsevers elsevers requested a review from noguchi-takahiro May 17, 2024 10:51
@elsevers
Copy link
Contributor

Great, thanks @wchan29! I have asked @noguchi-takahiro to give this a review and then I will take a look once you have an approval from him.

Copy link
Contributor

@noguchi-takahiro noguchi-takahiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @wchan29. Your machine constants analyzer is so useful, and working well. I have few requests. Please see comments below and consider if you could update them.

"Machine Suspension Force Constant [N/A]"
if self.problem.solve_Kf:
Is_list, force = self.Kf_data
Kf,_ = np.polyfit(Is_list,force,deg=1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering how we should define the force constant of $K_f$, especially in the case where the force saturates in terms of the current.

Here is the force vs current plot based on the data through this machine constants analyzer. You can see the force has linear characteristic, but saturates above 15 A.

Fxy_ixy

When we calculate the force constant, should we use polyfit via data considering the force saturation (resulting in $K_f=1.80$)? Or should we use the force data from only linear region (resulting in $K_f=1.98$)? What do you think @elsevers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elsevers elsevers mentioned this pull request May 30, 2024
Copy link
Contributor

@elsevers elsevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @wchan29. This is going to be super useful. I have been mulling this all over since our discussion, and I have some requests for changes to 1) documentation and 2) how users interface to this analyzer / problem.

Documentation

I'd like to see the Model Background section refined to indicate that in each of these expressions you are fitting a linear polynomial to torque (or force) data while current (or displacement) varies. Can you add that to your explanation and add a plot to illustrate this?

You'll also notice that the current documentation in this section has some type-o level mistakes. For example, after this expression
image
it refers to the expression telling the reader what $F_c$ means, but $F_c$ is not actually in the expression. Oops (no need to tell the reader about $F_c$).

Change definition of Back-EMF Constant

Instead of $\rm V RMS$ let's use amplitude/peak, to be consistent with $k_f$ and $k_t$

Changes to How Users Interface to this Analyzer / Problem

Instead of Kf_Kt_steps and Kphi_step, let's have the users provide a list of operating points to use for the evals.

Right now you have a clever time saving mechanism of solving for Kf and Kt at the same time. Let's cut that, and instead have a list of operating points for Kf and a list of operating points for Kt The linear polynomial fit will be based on these operating points. (Note that doing this will enable a user to chose over what range the linear fit will be applied because they can chose over what range to apply the current)

Right now the code splits these types of parameters between the problem and the analyzer. Instead, let's put it all in the problem (all the operating points, the JMAG_2D_Config, etc.).

Best Practice Enhancement

Ideally, the problem and analyzer would be structured so that the analyzer does not need to know anything about JMAG or the BPSM Machine Analyzer. Instead, your analyzer would call functions in the problem class to analyze at each operating point (and the problem class would then know how to take care of working with JMAG / BPSM Machine Analzyer). This would enable someone in the future to swap out this problem class for another problem class that uses Maxwell or MagNet or FEMM or anything...

It is okay with me if this "best practice enhancement" is not feasible right now. Instead, we can save it as an issue for a future upgrade.

@wchan29
Copy link
Contributor Author

wchan29 commented Jun 28, 2024

@elsevers Per discussion today, I have implemented your suggestion of implementing a list of operating point as input for evaluating each machine constant. now instead of taking number of steps it takes a list of BSPM_Machine_Oper_Pt objects.
In addition, changes has been made to the docs.

Copy link
Contributor

@elsevers elsevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the vector arrow on delta?
image

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.

5 participants