newville: @schachmett thanks very much for the thorough explanations!  I can believe these are all important and necessary changes, but I want to try this out myself too.  For example, I'm not sure why this is generating errors in `_getval`, but think that maybe we should figure that out.

Turning your very nice minimal example into a unit test should not be hard.   For example, it could be added to `tests/test_parameter.py` using `assert` on the size of the asteval error array. 

My inclination is to merge this soon. If you don't have time or energy to add tests in the code, we can do that soon afterwards.   

Anyone else have concerns or objections to that?

schachmett: @newville thank you. The errors in `_getval` are generated because of the `show_errors=False` argument to `_expr_eval`. When this option is set to `True`, the errors are printed (with some weird indentation) instead and `check_ast_errors` prints them a second time.

I think I can find the time for adding tests this week, probably today or tomorrow. So you could leave the PR open until the tests are committed?
schachmett: @newville Well, `show_errors=True` and the surrounding try, except could just be omitted at the cost of some duplicate printed output.

As for your proposed changes, they would effectively be the same as my first commit. The problems there are:
* It might be that the error array does not exist yet at this point in the code, raising an AttributeError on some occasions (That is why Travis CI failed on my first commit).
* Inside `_expr_eval.parse()`, an exception is stored in the error list and then raised. If this error is catched by the surrounding application, the list is not cleaned afterwards and other parts of the code (meaning `_getval()`) can raise the same exception again via `check_ast_errors()`. This is why I did not let `parse()` raise an error of its own and why I get rid of old exceptions in the `finally` clause. -> It is ensured that the error list is cleaned **after** raising and it is only raised once (which would be the whole point of `check_ast_errors()`)
* For `_getval`, essentially the same applies but here, other exceptions can be raised. `parse()` only checks for SyntaxErrors while calling `_expr_eval()` also gets nameerrors (see minimal example). That is why I wanted to call this once during `Parameter.set()`, so invalid expressions get detected right away. I don't know if this would break code where you set an expression for some parameter relating it to another not yet initialized parameter, or if that would already raise exceptions and is discouraged anyway. My code passing the nosetests indicates the latter.

This looked real simple to me, too, in the beginning and maybe I am overthinking it now. But I don't see any real harm in writing try, except clauses in places where errors are checked, anyway.
newville: On Wed, Aug 1, 2018 at 5:13 PM Simon Fischer <notifications@github.com>
wrote:

> @newville <https://github.com/newville> Well, show_errors=True and the
> surrounding try, except could just be omitted at the cost of some duplicate
> printed output.
>
> As for your proposed changes, they would effectively be the same as my
> first commit. The problems there are:
>
>    - It might be that the error array does not exist yet at this point in
>    the code, raising an AttributeError on some occasions (That is why Travis
>    CI failed on my first commit).
>
>
Except that it will exist if it explicitly set to `[]`.

>
>    - Inside _expr_eval.parse(), an exception is stored in the error list
>    and then raised. If this error is catched by the surrounding application,
>    the list is not cleaned afterwards and other parts of the code (meaning
>    _getval()) can raise the same exception again via check_ast_errors().
>    This is why I did not let parse() raise an error of its own and why I
>    get rid of old exceptions in the finally clause. -> It is ensured that
>    the error list is cleaned *after* raising and it is only raised once
>    (which would be the whole point of check_ast_errors())
>
>
Yes, if there is an error in the expression that will be retained until
explicitly cleared. And, if you do reset the expression, it *will* be
cleared.  If you do not reset the expression, there will be an error in the
expression, and it will not evaluate correctly until fixed.  So the
existence of a non-empty `_expr_eval.error`  means there is a serious
problem that will prevent parameter evaluation.

>
>    - For _getval, essentially the same applies but here, other exceptions
>    can be raised. parse() only checks for SyntaxErrors while calling
>    _expr_eval() also gets nameerrors (see minimal example).
>
>
For sure, `parse()` is only sensitive to syntax errors not to other runtime
exceptions.  That is unavoidable.  But, if you "clear errors" before
resetting the expression, that should work.  As you have it, with clearing
the error in `_getval()` will happen *much* more often, and may ignore
runtime errors, say during the fitting process.  I'm not sure that is a
good idea.


>    - That is why I wanted to call this once during Parameter.set(), so
>    invalid expressions get detected right away.
>
>
But doing
    0) clear errors
    1) parse
    2) check for errors
will reliably (and clearly) check for parse errors right where they happen.


>    - I don't know if this would break code where you set an expression
>    for some parameter relating it to another not yet initialized parameter, or
>    if that would already raise exceptions and is discouraged anyway. My code
>    passing the nosetests indicates the latter.
>
> This looked real simple to me, too, in the beginning and maybe I am
> overthinking it now. But I don't see any real harm in writing try, except
> clauses in places where errors are checked, anyway.
>

A small change that
addresses the actual problem where it actually happens (as opposed to
trying to fix other potential problems) is almost always preferable.


Please don't remove `check_ast_errors()`.

schachmett: Okay, it seems I really did overthink it. I will revert to the simple solution tomorrow as it is getting late here.

Do you however think that leaving _getval inside the set method to directly get the NameErrors when setting expr is a good idea?
reneeotten: @schachmett @newville sorry for being late to the party and it looks like the issue is almost fix already!

Two comments:
1) I am not sure that ```_getval``` should be called inside the function to set an expression. It seems that the purpose of this is to get a more direct/clear error message when using a variable in an expression that wasn't defined yet, correct? I would prefer to put ```check_ast_errors()``` back as @newville said earlier and explore if there is another way of doing this - I haven't checked carefully... so perhaps I'm mistaken!
2) once it's settled on what to do please squash the commits (probably into to two, one fixing the issue and another for adding the test) and use slightly more useful commit messages.
schachmett: hi @reneeotten, yes you are right about the more direct error message. Essentially I would feel like 
```python
with self.assertRaises(NameError):
    p1['y'].set(expr='z*5.0')
```
should be in the tests and therefore either `_getval` or or `self._expr_eval(self._expr_ast)` directly would need to be called somewhere in `__set_expression` or in `set`. The reason for this test is that it makes evaluation of user input in an application easier.

And oops, I just forgot about putting back `check_ast_errors` into `__set_expression` because the tests passed anyway with `_getval` being in there. If you @reneeotten and @newville could give me your opinion on the first paragraph I'd just implement it or not and then squash the commits like you said.
newville: @schachmett  I believe that `_getval()` should not be needed in `_set_expression()`.   That is, I believe the value will be accessed by `Parameters.set()` immediately afterwards and the NameError raised anyway.



diff --git a/lmfit/parameter.py b/lmfit/parameter.py
index 9a5c7d1a..235a919b 100644
--- a/lmfit/parameter.py
+++ b/lmfit/parameter.py
@@ -826,6 +826,8 @@ class Parameter(object):
         if val is None:
             self._expr_ast = None
         if val is not None and self._expr_eval is not None:
+            self._expr_eval.error = []
+            self._expr_eval.error_msg = None
             self._expr_ast = self._expr_eval.parse(val)
             check_ast_errors(self._expr_eval)
             self._expr_deps = get_ast_names(self._expr_ast)

asteval errors are not flushed after raising
#### Description

With parameter.set(expr="foo"), if the expression raises an exception in asteval, this error is stored in parameter._expr_eval.errors. Now, the function https://github.com/lmfit/lmfit-py/blob/6c87262fcfd3c361b197c6769852f76366113246/lmfit/parameter.py#L18-L21 gets called from parameter.__set_expression(val) and checks if the length of this errors variable is longer than 0 and if it is, the exception is raised.

However, if you try to set the expression for this parameter again (to a VALID expression), the Parameter._expr_eval.errors variable will still contain the first exception and so, check_ast_errors() will always raise an Exception. This should not be the expected behaviour, for example if lmfit is used from a GUI and the user tries to enter a valid expression after first entering an invalid one (where the exception was caught).

I will submit a PR where check_ast_errors() flushes the parameter._expr_eval.errors before trying to set a new expression.


###### Version information
Python: 3.6.5 (default, Apr  1 2018, 05:46:30) 
[GCC 7.3.0]
lmfit: 0.9.11, scipy: 1.1.0, numpy: 1.15.0, asteval: 0.9.12, uncertainties: 3.0.2, six: 1.11.0

###### Minimal example
```python
import lmfit

def set_constraints(paramname, expr):      
#        params[paramname]._expr_eval.error.clear() # this would be the dirty fix
        try:
            params[paramname].set(expr=expr)
            print("expr was valid")
        except SyntaxError:
            params[paramname].set(expr="")
            print("expr was not valid")
model = lmfit.models.PseudoVoigtModel()
params = model.make_params()
set_constraints("amplitude", expr="sigma * 2")
set_constraints("amplitude", expr="fail *")
set_constraints("amplitude", expr="sigma * 2")
```
produces:
```python
expr was valid
expr was not valid
expr was not valid
```
If you uncomment the "dirty fix", it produces
```python
expr was valid
expr was not valid
expr was valid
```
diff --git a/tests/test_parameters.py b/tests/test_parameters.py
index 73604e47..7344241d 100644
--- a/tests/test_parameters.py
+++ b/tests/test_parameters.py
@@ -212,6 +212,23 @@ class TestParameters(unittest.TestCase):
         pars.add('c4', expr='csum-c1-c2-c3', min=0, max=1)
         assert_(isclose(pars['c4'].value, 0.2))
 
+    def test_invalid_expr_exceptions(self):
+        "test if an exception is raised for invalid expressions (GH486)"""
+        p1 = Parameters()
+        p1.add('t', 2.0, min=0.0, max=5.0)
+        p1.add('x', 10.0)
+        with self.assertRaises(SyntaxError):
+            p1.add('y', expr='x*t + sqrt(t)/')
+        assert(len(p1['y']._expr_eval.error) > 0)
+        p1.add('y', expr='x*t + sqrt(t)/3.0')
+        p1['y'].set(expr='x*3.0 + t**2')
+        assert('x*3' in p1['y'].expr)
+        assert(len(p1['y']._expr_eval.error) == 0)
+        with self.assertRaises(SyntaxError):
+            p1['y'].set(expr='t+')
+        assert(len(p1['y']._expr_eval.error) > 0)
+        assert_almost_equal(p1['y'].value, 34.0)
+
 
 if __name__ == '__main__':
     unittest.main()

pip install -e .[dev]
parse_log_pytest
requirements.txt
nose pytest
3.6
requirements.txt
pytest --no-header -rA --tb=line --color=no -p no:cacheprovider -W ignore::DeprecationWarning
asteval==0.9.26
attrs==22.2.0
certifi==2021.5.30
future==1.0.0
importlib-metadata==4.8.3
iniconfig==1.1.1
-e git+https://github.com/lmfit/lmfit-py.git@6c87262fcfd3c361b197c6769852f76366113246#egg=lmfit
nose==1.3.7
numpy==1.19.5
packaging==21.3
pluggy==1.0.0
py==1.11.0
pyparsing==3.1.4
pytest==7.0.1
scipy==1.5.4
six==1.17.0
tomli==1.2.3
typing_extensions==4.1.1
uncertainties==3.1.7
zipp==3.6.0

name: lmfit-py
channels:
  - defaults
  - https://repo.anaconda.com/pkgs/main
  - https://repo.anaconda.com/pkgs/r
  - conda-forge
dependencies:
  - _libgcc_mutex=0.1=main
  - _openmp_mutex=5.1=1_gnu
  - ca-certificates=2025.2.25=h06a4308_0
  - certifi=2021.5.30=py36h06a4308_0
  - ld_impl_linux-64=2.40=h12ee557_0
  - libffi=3.3=he6710b0_2
  - libgcc-ng=11.2.0=h1234567_1
  - libgomp=11.2.0=h1234567_1
  - libstdcxx-ng=11.2.0=h1234567_1
  - ncurses=6.4=h6a678d5_0
  - openssl=1.1.1w=h7f8727e_0
  - pip=21.2.2=py36h06a4308_0
  - python=3.6.13=h12debd9_1
  - readline=8.2=h5eee18b_0
  - setuptools=58.0.4=py36h06a4308_0
  - sqlite=3.45.3=h5eee18b_0
  - tk=8.6.14=h39e8969_0
  - wheel=0.37.1=pyhd3eb1b0_0
  - xz=5.6.4=h5eee18b_1
  - zlib=1.2.13=h5eee18b_1
  - pip:
      - asteval==0.9.26
      - attrs==22.2.0
      - future==1.0.0
      - importlib-metadata==4.8.3
      - iniconfig==1.1.1
      - nose==1.3.7
      - numpy==1.19.5
      - packaging==21.3
      - pluggy==1.0.0
      - py==1.11.0
      - pyparsing==3.1.4
      - pytest==7.0.1
      - scipy==1.5.4
      - six==1.17.0
      - tomli==1.2.3
      - typing-extensions==4.1.1
      - uncertainties==3.1.7
      - zipp==3.6.0
prefix: /opt/conda/envs/lmfit-py

tests/test_parameters.py::TestParameters::test_invalid_expr_exceptions
tests/test_parameters.py::TestParameters::test_add_many_params tests/test_parameters.py::TestParameters::test_copy tests/test_parameters.py::TestParameters::test_copy_function tests/test_parameters.py::TestParameters::test_deepcopy tests/test_parameters.py::TestParameters::test_dumps_loads_parameters tests/test_parameters.py::TestParameters::test_expr_and_constraints_GH265 tests/test_parameters.py::TestParameters::test_expr_was_evaluated tests/test_parameters.py::TestParameters::test_expr_with_bounds tests/test_parameters.py::TestParameters::test_isclose tests/test_parameters.py::TestParameters::test_pickle_parameter tests/test_parameters.py::TestParameters::test_pickle_parameters tests/test_parameters.py::TestParameters::test_set_symtable
lmfit/parameter.py
lmfit/parameter.py