add evaluation of dely for components of composite models #826
Conversation
Codecov Report
@@ Coverage Diff @@
## master #826 +/- ##
=======================================
Coverage 93.71% 93.71%
=======================================
Files 9 9
Lines 3483 3500 +17
=======================================
+ Hits 3264 3280 +16
- Misses 219 220 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
thanks @newville, this looks useful! I will take a look at the code changes later today/this week. I'd prefer to keep this PR focused on one topic; the Azure stuff shouldn't be part of this as it always takes a bit of time before the GitHub ecosystem and packages are available. If you don't mind I will take care of that once it's ready. |
|
@reneeotten OK, I don't disagree on the azure stuff - I was mostly testing it here and seeing if I could understand what was failing. I would be fine with treating this branch as "research/suggestions", and making a new PR with only the |
|
@reneeotten Any thoughts on this change in the code (separate from azure changes)? Should this be re-formed into 2 new, separate PRs? |
|
sorry @newville - I'll get to this tonight! |
There was a problem hiding this comment.
@newville sorry for the delay, still don't have internet at home... Two minor questions for this PR.
I'll take a look at Azure now and the pre-commit failure in the other PR and if I get that to work, I'll push that and we can drop all of that from this PR. Don't worry about doing that git magic, I'll take care of these trivialities!
| @@ -266,6 +267,10 @@ def __init__(self, func, independent_vars=None, param_names=None, | |||
| """ | |||
| self.func = func | |||
| self._prefix = prefix | |||
| if prefix is not None and len(prefix) > 0: | |||
There was a problem hiding this comment.
can this not just be if prefix:?
| @@ -266,6 +267,10 @@ def __init__(self, func, independent_vars=None, param_names=None, | |||
| """ | |||
| self.func = func | |||
| self._prefix = prefix | |||
| if prefix is not None and len(prefix) > 0: | |||
| if not valid_symbol_name(prefix + 'a'): | |||
There was a problem hiding this comment.
why do you need to add a to the prefix string here and test that; seems to me that just testing if prefix is valid should be sufficient?
There was a problem hiding this comment.
Yeah, this is kind of confusing.
prefixneeds to be a string.- it can be '' -- that's even the default (it is not a valid symbol name).
- if it is longer than length(1) it also needs to be that prefix+simple_variable_name is a valid symbol. I think that is identical to "prefix is a valid symbol name".
Some True values will not meet that. So, maybe
self.func = func
if not isinstance(prefix, str) :
prefix = ''
if len(prefix) > 0 and not valid_symbol_name(prefix):
raise ValueError(...)
self._prefix = prefix
| @@ -1497,6 +1502,9 @@ def eval_uncertainty(self, params=None, sigma=1, **kwargs): | |||
| < 1, it is interpreted as the probability itself. That is, | |||
| ``sigma=1`` and ``sigma=0.6827`` will give the same results, | |||
| within precision errors. | |||
| 3. Also sets attributes of `dely` for uncertainty of the model (same as return value) | |||
There was a problem hiding this comment.
what do you mean by "same as return value" - same type (i.e., numpy.ndarray) as return value, or...?
There was a problem hiding this comment.
same value. In the current version, the method returns an array. That return value should not break, but it's a little dely for each component as a side-effect, but not set dely for the full thing. So this will now do both. We could deprecate the return value, but it does not seem necessary.
|
@newville once you've had a chance to respond to the few questions here I'll disentangle the Azure from the "real" PR content and we can get this merged |
|
replaced by #831 |
Description
This adds calculation of
delyfor model components toModelResult.eval_uncertainties, addressing #761Type of Changes
Tested on
Python: 3.9.13 | packaged by conda-forge | (main, May 27 2022, 17:00:52)
[Clang 13.0.1 ]
lmfit: 1.0.3.post72+g1a8f035.d20221108, scipy: 1.9.0, numpy: 1.23.2, asteval: 0.9.28, uncertainties: 3.1.7
Verification
Have you