Fix bug in Kinetic normalization when pressure is present, add warning for this case to profile setters#2046
Fix bug in Kinetic normalization when pressure is present, add warning for this case to profile setters#2046
Conversation
dpanici
requested review from
a team,
YigitElma,
ddudt,
f0uriest,
rahulgaur104 and
unalmis
and removed request for
a team
…e scales using them
Memory benchmark result| Test Name | %Δ | Master (MB) | PR (MB) | Δ (MB) | Time PR (s) | Time Master (s) |
| -------------------------------------- | ------------ | ------------------ | ------------------ | ------------ | ------------------ | ------------------ |
test_objective_jac_w7x | 5.42 % | 3.853e+03 | 4.062e+03 | 208.67 | 40.80 | 36.25 |
test_proximal_jac_w7x_with_eq_update | 0.29 % | 6.619e+03 | 6.639e+03 | 19.17 | 165.64 | 165.94 |
test_proximal_freeb_jac | -0.02 % | 1.319e+04 | 1.318e+04 | -2.14 | 86.35 | 85.66 |
test_proximal_freeb_jac_blocked | 0.09 % | 7.463e+03 | 7.469e+03 | 6.48 | 73.81 | 74.41 |
test_proximal_freeb_jac_batched | -0.07 % | 7.459e+03 | 7.454e+03 | -5.32 | 74.01 | 75.22 |
test_proximal_jac_ripple | 0.37 % | 3.479e+03 | 3.491e+03 | 12.85 | 67.92 | 68.43 |
test_proximal_jac_ripple_bounce1d | -2.35 % | 3.644e+03 | 3.558e+03 | -85.67 | 75.43 | 77.65 |
test_eq_solve | 0.94 % | 2.006e+03 | 2.025e+03 | 18.79 | 94.19 | 95.10 |For the memory plots, go to the summary of |
…e warning if pressure and kinetic prfiles are assigned in the normalization
…ESC into dp/kinetic-normalization
… setting kinetic profiles
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2046 +/- ##
=======================================
Coverage 94.52% 94.53%
=======================================
Files 102 102
Lines 28785 28798 +13
=======================================
+ Hits 27210 27224 +14
+ Misses 1575 1574 -1
🚀 New features to boost your workflow:
|
desc/vmec_utils.py
Outdated
| "S" + str(file.dimensions["dim_00100"].size), | ||
| ) | ||
| ), | ||
| encoding="ascii", |
There was a problem hiding this comment.
Ah yea I likely had accidentally added that here
desc/objectives/normalization.py
Outdated
| has_kinetic = False | ||
| if ( | ||
| thing.electron_density is not None | ||
| and thing.electron_temperature is not None | ||
| and thing.atomic_number is not None | ||
| and thing.ion_temperature is not None | ||
| ): |
There was a problem hiding this comment.
We should split this up like:
- if thing has either temperature profile, add the temperature scaling
- if thing has a density profile, add the density scaling
- if thing has a pressure profile, add the pressure scaling
And have them all be independent of each other. It might not matter now, but in #2090 some but not all of the kinetic profiles can be None which would break this check.
There was a problem hiding this comment.
Have we decided that #2090 is the way we will be doing this? I mean right now, the profile behavior is not well-defined when there is a subset. It makes little sense for these to be independent. Even in that PR change, I think that there should still be some sort of warning, or you can make the warning specifically consider the case you mention (which I think is that ion density profile may exist but no Zeff or vice versa?)
There was a problem hiding this comment.
I think this is the most I can split up without making the logic more complex, let me know what you think
| warnif( | ||
| has_kinetic, | ||
| UserWarning, | ||
| "Equilibrium has both kinetic and pressure profiles assigned to it." | ||
| " By default, the pressure profile will be used for the normalization " | ||
| "and the computation of pressure, which may lead to unexpected " | ||
| "results when kinetic profiles are also present.", | ||
| ) |
There was a problem hiding this comment.
I think this warning is a little unnecessary since a similar warning will have already been thrown when the equilibrium is initialized or the profiles were assigned.
There was a problem hiding this comment.
I think it is necessary as it may be unclear which is used for normalization, like if you are using pressure and kinetic profiles but one is inconsistent with the other. I want to make it very clear what is happening in the case of both pressure and kinetic assigned
4 participants
Resolved #1995