736 warnings for function ncore #3060

chris-ashe · 2024-02-15T10:46:11Z

Description

The logging of the ncore error to the terminal during iterations has been removed and replaced with a level 2 error that is shown at the end of the run. This warns the user to raise the lower bound of dene to prevent VMCON going near negative calculated values of n0. This is only shown when the run is finished.

Documentation has been added warning the user to use constraint 81 to prevent un-realistic profiles and to be weary about setting the lower bound of dene too low.

The bounds in iteration_variables.f90 for fgwped, fgwsep & dene have been varied to represent the values found in realistic solutions.

Long term solution is being discussed in #3059

Checklist

I confirm that I have completed the following checks:

I have justified any large differences in the regression tests caused by this pull request in the comments.
I have added new tests where appropriate for the changes I have made.
If I have had to change any existing unit or integration tests, I have justified this change in the pull request comments.
If I have made documentation changes, I have checked they render correctly.
I have added documentation for my change, if appropriate.

mkovari · 2024-02-15T14:33:12Z

I would still like the warning to be explicit about what sort of lower and upper bounds people should set.

For example:
dene Recommended value of boundl(6) > 80% of expected volume-averaged density.

It might also help to narrow the default bounds as follows.

Separatrix density

PROCESS/source/fortran/iteration_variables.f90

Lines 3378 to 3380 in b8c0a3d

    
           lablxc(152) = 'fgwsep        ' 
        
           boundl(152) = 0.001D0 
        
           boundu(152) = 1.000D0

Change to:

    lablxc(152) = 'fgwsep        '
    boundl(152) = 0.001D0
    boundu(152) = 0.1D0

Pedestal density

    lablxc(145) = 'fgwped        '
    boundl(145) = 0.500D0
    boundu(145) = 1.000D0

Change to

    lablxc(145) = 'fgwped        '
    boundl(145) = 0.100D0
    boundu(145) = 0.75D0

    lablxc(6) = 'dene          '
    boundl(6) = 1.00D19
    boundu(6) = 1.00D21

Change to

    lablxc(6) = 'dene          '
    boundl(6) = 2.00D19
    boundu(6) = 1.00D21

timothy-nunn

@chris-ashe if you answer my comment I will approve it subject to your reply

timothy-nunn · 2024-02-20T09:59:34Z

process/profiles.py

+        if ncore < 0.0:
+            # Allows solver to continue and
+            # warns the user to raise the lower bound on dene if the run did not converge
+            error_handling.report_error(282)


What was the reason this was not done before? What was the point of the kludge if the solver can deal with a negative ncore? I always thought this was to stop NaNs further down the code

Running the files it dosent seem to make a difference as they all fail eventually anyway. It seems better to have it fail quickly and the user updates the lower bound to prevent it going negative again in future runs

I probably need @jonmaddock to comment on this, as he is the one that wrote the kludge. For now, since this is a level 2 error, I will approve it but will make a follow-up issue to check that this fix is ok with the UQ tools (which is why the kludge was added)

If needs be we could put the kludge back in without adding back the logger output as it just slows down the terminal output

Probably a good idea, and then #3059 can address making this model more robust

timothy-nunn · 2024-02-20T10:04:34Z

source/fortran/iteration_variables.f90

@@ -3377,7 +3377,7 @@ subroutine init_itv_152
    implicit none
    lablxc(152) = 'fgwsep        '
    boundl(152) = 0.001D0
-    boundu(152) = 1.000D0
+    boundu(152) = 0.1D0
  end subroutine init_itv_152


I am a little concerned about changing the default bounds as this could lead to existing input files (that use the default bounds) starting outside of the feasible region. In this instance, the 3 changes are minor so I think I am happy with it.

The output values in the large tokamak regression are 0.85 and 0.5 for pedestal and separatrix respectively. @mkovari
Which is outside the bounds recommended

Output values? Have you calculated neped/nGW and nesep/nGW from the data in the output file?

Electron density at separatrix / nGW (fgwsep_out) 5.000E-01
Electron density at pedestal / nGW (fgwped_out) 8.500E-01

From the output file of the large tokamak regression test

I would still like the warning to be explicit about what sort of lower and upper bounds people should set.

For example:
dene Recommended value of boundl(6) > 80% of expected volume-averaged density.

Is this true for all cases? Even for a stellerator?
The output warning are limited to one line so I will need to amend the current one as I cant add it under unless we change their length

Maybe point people to a section in the documentation with the warning? And have a longer explanation in the docs.

mkovari · 2024-02-20T10:49:47Z

With all due respect, I find this approach slightly annoying. Wouldn't it be best to agree first on what should be done?

chris-ashe · 2024-02-20T10:52:33Z

@mkovari This is a temporary fix for papercuts and a more robust future solution can be discussed in #3059

mkovari · 2024-02-20T11:00:31Z

So what has actually changed?

chris-ashe · 2024-02-20T11:04:03Z

Kludging ncore is no longer spammed to the terminal when PROCESS is iterating.
A level 2 error is raised if ncore now goes negative telling the user to raise the value of dene and or its lower bound.
Updates to the pedestal documentation about how to set up a file with constraint 81

timothy-nunn · 2024-02-20T11:26:20Z

I agree that the solution to this issue and the papercut is to remove the warning "spam" and providing help in the documentation. Subsequent discussions can decide a long-term solution to our profiles and will be able to consider how best we can stop unphysical profiles from occurring.

chris-ashe · 2024-02-20T11:46:30Z

@mkovari Are you happy with the changes as of now that provide a route to solving the negative problems by pointing to the documentation and raising new warning?
We can discuss a longer term and more robust solution with @jonmaddock that will work in a range of different scenarios that doesn't affect convergence.

mkovari · 2024-02-20T11:50:50Z

I am happy in the sense that I don't think these changes are going to make the situation any worse.

* ncore warning * syntax fix * return ncore * change error type * tidy up * update docs * updated bounds of pedestal params * add ncore = 1E-6 back in * re-update pedestal bounds

chris-ashe added 7 commits February 14, 2024 11:29

ncore warning

167b650

Merge branch 'main' into 736-warnings-for-function-ncore

355e43c

syntax fix

3b33e4d

return ncore

2ba076e

change error type

bf25b7c

tidy up

52fd0de

update docs

cc821c0

chris-ashe added the Pedestal label Feb 15, 2024

chris-ashe requested a review from timothy-nunn February 15, 2024 10:46

chris-ashe self-assigned this Feb 15, 2024

chris-ashe linked an issue Feb 15, 2024 that may be closed by this pull request

Warnings for function ncore #736

Closed

chris-ashe changed the title ~~736 warnings for function ncore~~ WIP:736 warnings for function ncore Feb 15, 2024

updated bounds of pedestal params

9f319f4

chris-ashe changed the title ~~WIP:736 warnings for function ncore~~ 736 warnings for function ncore Feb 20, 2024

timothy-nunn reviewed Feb 20, 2024

View reviewed changes

timothy-nunn mentioned this pull request Feb 20, 2024

Level 2 warning instead of kludge/STOP1 for negative ncore #3065

Closed

chris-ashe added 2 commits February 20, 2024 10:37

re-update pedestal bounds

229fdda

timothy-nunn approved these changes Feb 20, 2024

View reviewed changes

timothy-nunn merged commit 51307a5 into main Feb 20, 2024

timothy-nunn deleted the 736-warnings-for-function-ncore branch February 20, 2024 12:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

736 warnings for function ncore #3060

736 warnings for function ncore #3060

736 warnings for function ncore #3060

736 warnings for function ncore #3060

Conversation

Description

Checklist

Separatrix density

Pedestal density

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment