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

RZ stopping criteria #385

Merged
merged 11 commits into from
Dec 29, 2023
Merged

RZ stopping criteria #385

merged 11 commits into from
Dec 29, 2023

Conversation

ejpaul
Copy link
Contributor

@ejpaul ejpaul commented Dec 13, 2023

This pull request implements new StoppingCriteria based on minimum and maximum values of the cylindrical R and Z coordinates. This is more convenient to use for creating Poincare plots of very compact configurations, for which the definition of a bounding surface may be challenging. The new classes are called RStoppingCriteria and ZStoppingCriteria, which are defined by the critical value of R or Z and a boolean that indicates whether the minimum or maximum is evaluated. The MinToroidalFluxStoppingCriteria and MaxToroidalFluxStoppingCriteria have been replaced by ToroidalFluxStoppingCriteria to mimic this structure.

@ejpaul ejpaul requested a review from mbkumar December 13, 2023 20:19
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (50f41d6) 91.43% compared to head (01aac45) 91.44%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #385   +/-   ##
=======================================
  Coverage   91.43%   91.44%           
=======================================
  Files          73       73           
  Lines       12709    12717    +8     
=======================================
+ Hits        11621    11629    +8     
  Misses       1088     1088           
Flag Coverage Δ
unittests 91.44% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@landreman landreman left a comment

Choose a reason for hiding this comment

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

Kindly add a unit test in which RStoppingCriterion and ZStoppingCriterion are called.

@mbkumar
Copy link
Collaborator

mbkumar commented Dec 17, 2023

My 2 cents:

Replacing MinToroidalFluxStoppingCriteria and MaxToroidalFluxStoppingCriteria with ToroidalFluxStoppingCriteria is breaking the existing API. I am not sure who is using this piece of code, but to favor the users, can you retain the previous two classes and call the ToroidalFluxStoppingCriteria behind the scenes whenever the user calls the [Max,Min]ToroidalFluxStoppingCriteria with a deprecation warning? Please make sure the existing tests are retained.

@ejpaul
Copy link
Contributor Author

ejpaul commented Dec 27, 2023

@mbkumar @landreman Thanks for your feedback. I have added a unit test to test_fieldline.py, and I have reverted to the previous API.

@landreman landreman merged commit 6b203fd into master Dec 29, 2023
47 checks passed
@ejpaul ejpaul deleted the rz_stopping_criteria branch January 10, 2024 01:16
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.

3 participants