Matlab test #66
Matlab test #66
Conversation
|
Impressive work, @dekuenstle : Kudos! |
# Conflicts: # psignifit/psigniplot.py
There was a problem hiding this comment.
Reviewed with @otizonaizit , let's discuss the open questions
psignifit/_configuration.py
Outdated
| @classmethod | ||
| def from_matlab_options(cls, option_dict: Dict[str, Any], **kwargs): | ||
| python_configs = config_from_matlab(option_dict, **kwargs) | ||
| return cls(**python_configs) |
There was a problem hiding this comment.
This should be a helper function in the MATLAB tests
There was a problem hiding this comment.
I removed this class function but exposed psignifit.config_from_matlab. This might be handy for users switching from MATLAB to Python
psignifit/_matlab.py
Outdated
There was a problem hiding this comment.
this file should go to the tests folder
There was a problem hiding this comment.
See above -> Matlab compatiblitiy can be useful for people switching from matlab
# Conflicts: # psignifit/_configuration.py # psignifit/psignifit.py # psignifit/psigniplot.py
# Conflicts: # psignifit/tests/test_param_recovery.py
…om_matlab_options
|
|
I dug deeper into the difference between the MATLAB and psignifit results on these datasets and could resolve many.
Still, the tests fail for some datasets which needs further investigation.
|
| for parameter, prior in priors.items(): | ||
| priors[parameter] = normalize_prior(prior, bounds[parameter]) | ||
| for parameter, bound in bounds.items(): | ||
| priors[parameter] = normalize_prior(priors[parameter], bound) | ||
| return priors |
There was a problem hiding this comment.
I am not sure I understand why this was changed...
There was a problem hiding this comment.
I assume this is a relict from a time when bounds might not be defined for fixed items. Therefore, the former version would crash. If I'm right, you changed the behavior last week such that for fixed parameters still bounds are added in the psignifit funciton?!
| if 'gamma' not in fit_dict: # equal asymptotes | ||
| fit_dict['gamma'] = fit_dict['lambda'] | ||
| intervals_dict['gamma'] = intervals_dict['lambda'] |
There was a problem hiding this comment.
this is taken care of, at least partially in the following lines (106-110), so line 101
intervals_dict['gamma'] = intervals_dict['lambda'] should be added after line 110 below
There was a problem hiding this comment.
Historically this PR grew too big and does too many things. I currently see it more as a playground to discover if/why there are differenes between the MATLAB and Python version. Afterwards IMHO the essence of this PR should be reimplemented in a separate branch.
There was a problem hiding this comment.
Historically this PR grew too big and does too many things. I currently see it more as a playground to discover if/why there are differenes between the MATLAB and Python version. Afterwards IMHO the essence of this PR should be reimplemented in a separate branch.
OK. But then it would be useful to get rid of the spurious changes and then merge main into this branch. I understand the playground idea, but given that we are changing things in main that may be relevant here, we need some form of synchronization, I think... Or, really create a new branch and continue the playground there. I am just afraid that at some point in the future no one will remember which changes are relevant and which ones should be discarded...
No description provided.