Skip to content
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

Better way to deal with an equilibrium having both an iota and current profile assigned to it #1482

Closed
dpanici opened this issue Dec 19, 2024 · 2 comments · Fixed by #1521
Closed
Assignees
Labels
enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc P2 Medium Priority, not urgent but should be on the near-term agend

Comments

@dpanici
Copy link
Collaborator

dpanici commented Dec 19, 2024

Currently, when we have an equilibrium with both a current and iota profile assigned to it, we will throw an error when doing a default eq.solve for instance, because it will try to assign FixCurrent and FixIota as profile constraints.

However, one may still do things like eq.compute or even do an optimization with lsq-auglag (as that does not trigger an equilibrium solve). This leads to weird behavior where the equilibrium seems ok, but it is not using its assigned current profile at all and is instead using its iota profile (as we check for iota profile existing first in our compute functions for iota).

This is in some sense really just due to user error, they should know not to have both a current and iota profile attached, but when a user loads a wout file, by default the iota profile is used, and if they then assign a current profile to it without first setting eq.iota=None, then they may not realize they've made a mistake here. We could try to add a warning when a current/iota profileis assigned while the other profile attribute exists, and say UserWarning: Equilibrium already has existing current profile. Please remove with eq.current=None before using for optimization or equilibrium solving

Or, I would even be ok with automatically removing the other profile, as if I am assigning a current profile to an equilibrium, I almost certainly don't want the iota profile to still be attached to the equilibrium (and in most cases it will cause problems later). We could do this verbosely and still throw a warning saying UserWarning: Adding an iota profile to Equilibrium which already has existing current profile. Removing the Equilibrium current profile, as only one of iota/current is allowed to be assigned to an equilibrium.

I favor the latter option, but what do you guys think? @PlasmaControl/desc-dev

@dpanici dpanici added enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc P2 Medium Priority, not urgent but should be on the near-term agend labels Dec 19, 2024
@dpanici dpanici self-assigned this Dec 19, 2024
@dpanici dpanici changed the title Better deal with an equilibrium having both an iota and current profile assigned to it` Better way to deal with an equilibrium having both an iota and current profile assigned to it` Dec 19, 2024
@YigitElma
Copy link
Collaborator

I think the second option is nicer. I like things to be automatic as long as it is verbose about it.

@dpanici dpanici changed the title Better way to deal with an equilibrium having both an iota and current profile assigned to it` Better way to deal with an equilibrium having both an iota and current profile assigned to it Dec 20, 2024
@Mandelbr0t
Copy link
Collaborator

I also favor the second option.

YigitElma added a commit that referenced this issue Jan 21, 2025
Improve handling of Assigning profiles to avoid equilibria having both
iota and current assigned.

Resolves #1482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General label for enhancement. Please also tag with "Speed", "Interface", "Functionality", etc P2 Medium Priority, not urgent but should be on the near-term agend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants