-
Notifications
You must be signed in to change notification settings - Fork 75
Part I of force_and_torque_overhaul breakup: Update of permanent magnet helpers, moving coil optimization functionality to its own helper file #558
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: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
==========================================
+ Coverage 92.56% 92.84% +0.27%
==========================================
Files 83 84 +1
Lines 16229 16509 +280
==========================================
+ Hits 15023 15327 +304
+ Misses 1206 1182 -24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@mishapadidar Friendly reminder! |
mishapadidar
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 for the review reminder!
I am a still a bit confused on the use case of coil_optimization_helper_functions. The module holds mostly convenience functions for building QFMs or running stage 2 optimization. Currently, some of the functions seem to be built with a particular use case or run paradigm in mind. If these features are intended to be user facing, it would be beneficial to make the functions general enough, so users can get the most out of them. Part of my confusion with these functions is that they seem very specific how they are intended to be used or run. For example, there are a few functions to perform optimizations of various types i.e. initial_optimizations_QH, initial_optimizations. It would be best to consolidate these as much as possible.
A general concern I have with the convenience functions is that some of them are designed for a particular workflow rather than being designed for expanding functionality. We want to build our library to expose building blocks, rather than particular workflows. Typically, we relegate workflows to example scripts or tutorials, since adding them to the code breaks consistency: why should some workflows get “special” shortcuts while others don’t. On the other hand, some routines are highly used and might deserve a special convenience function. For example, vacuum stage two optimization solves a common problem. In that sense, maybe it makes sense to package it up. However, when doing so, the method should be built in the same style as a modular "building block" (BoozerSurface is a good example of this).
Another point is that some of the functions write files out, without explicitly stating that they will do so in the documentation. See principle of least astonishment. In general, we should have two functions: one to do the computation, and another to write the files. Having both in one may be confusing to users, and makes testing/maintainability more difficult.
My final comment is that some of these functions may belong in other modules. For example, make_qfm could be in field or geo. Why not just move them now.
For concrete to-dos:
- Lets try to consolidate the various functions that run stage two as much as possible, and remove any that are too "use-case specific".
- Try to avoid writing files unless.
- Move files from the helpers to a better home when possible.
| bs = BiotSavart(coils) | ||
| bs.set_points(s.gamma().reshape((-1, 3))) | ||
| else: | ||
| path = glob.glob(f"../**/{UUID_init_from}/biot_savart.json", recursive=True)[0] |
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.
Why so much saving/loading?
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.
Allows one to continue from previous optimization results
…ion and reduced redundancy of the functions.
|
@mishapadidar Just made the recommended changes. Didn't move anything to geo/field yet since I'm not sure of the best choice for this. Consolidated and generalized the functions as you mentioned. I think the benefit of functions like "vacuum_stage_II_optimization" is that in principle we can stop having everyone rewrite their own bespoke coil optimization code and just reuse these helper functions. It would be nice in SIMSOPT if run scripts were more like 20 lines of python code instead of 200, much of which is duplicated (often without thinking about it) from previous scripts. |
… of the helper functions. Got the tests and examples running well again.
|
As far as I can tell, failing tests seem to be a SPEC installation problem |
I have separated out a substantial piece of PR #509 that relates to adding useful coil optimization functionality to its own util file, as well as reducing the time spent in running the permanent magnet examples, and otherwise reorganizing some functions between the permanent magnet and coil helper functions in utils/.
I would like to get this merged in main before continuing onwards to Part II. Note that code coverage is not perfect only because I have some functionality waiting to be tested once the force and torque terms are back in there.