Hi Troels,
Considering how easy this line of code could cause issues in the
future, do we now test this within the test suite? Would you like to
create such a system test to make sure it is now properly working for
your analysis? A good way might be to loop over all dispersion models
creating a data pipe for each, set up a small system of two clustered
spins, loop over all parameters of the model and set the parameter
values to the defaults, then call the assemble_parameter_vector()
function and check that the parameter array is what you would expect?
Such a test would eliminate all related dx.map user function problems.
What do you think? It would make this part of the dispersion analysis
much, much more robust.
Also, maybe this list of 'global parameters' could be shifted into
lib.dispersion.variables to make it easier to maintain. It could also
have 'k_BC' and 'k_AC' added for completeness. And I can clearly see
that 'pB' and 'pC' are also missing, so the models with these
parameters will also fail miserably here! Shifting into
lib.dispersion.variables might make it easier to test too, as unit
tests might work better. You could take the parameter lists out of
this module, for example MODEL_PARAMS_NS_R1RHO_3SITE = ['r2*', 'pA',
'dw_AB', 'kex_AB', 'pB', 'dw_BC', 'kex_BC', 'kex_AC'], remove the
'r1', 'r2', 'dw*', and 'phi_ex*' parameters and then check that the
remaining parameters are all located within the global parameter list.
That would be significantly easier. Though it would be harder to
maintain compared to a system test looping over all models, as new
models would not be automatically tested.
One last thing, I can see a little lower down the method that the code
for setting the R10 and R20 parameters are identical! I must have
missed this in one of your earlier commits for R1 fitting support. To
avoid future bugs, I would strongly recommend that you merge these two
sections. Simply do a test of "if param_i in PARAMS_R20 + ['r1']:".
Code duplication like this should be avoided at all costs, as it is an
important source of future bugs.
Cheers,
Edward
On 7 October 2014 21:35, <tlinnet@xxxxxxxxxxxxx> wrote:
Author: tlinnet
Date: Tue Oct 7 21:35:03 2014
New Revision: 26197
URL: http://svn.gna.org/viewcvs/relax?rev=26197&view=rev
Log:
Critical fix, for k_AB not belonging to list of global parameters.
k_AB was only changed to the spin of interest, but not for the rest of the
cluster.
When the parameter vector is assembled,
"assemble_param_vector(spins=spins)" it takes the global parameter from
spin 0.
Bug #22754 (https://gna.org/bugs/index.php?22754): The minimise.calculate()
does not calculate chi2 value for clustered residues.
Modified:
trunk/specific_analyses/relax_disp/api.py
Modified: trunk/specific_analyses/relax_disp/api.py
URL:
http://svn.gna.org/viewcvs/relax/trunk/specific_analyses/relax_disp/api.py?rev=26197&r1=26196&r2=26197&view=diff
==============================================================================
--- trunk/specific_analyses/relax_disp/api.py (original)
+++ trunk/specific_analyses/relax_disp/api.py Tue Oct 7 21:35:03 2014
@@ -1192,7 +1192,7 @@
raise RelaxError("The parameter '%s' is not valid for
this data pipe type." % param_i)
# If the parameter is a global parameter, then change for
all spins part of the cluster.
- if param_i in ['pA', 'kex', 'tex', 'kB', 'kC', 'kex_AB',
'kex_BC', 'kex_AC']:
+ if param_i in ['pA', 'kex', 'k_AB', 'tex', 'kB', 'kC',
'kex_AB', 'kex_BC', 'kex_AC']:
selection_list = spin_ids
else:
selection_list = [spin_id]
_______________________________________________
relax (http://www.nmr-relax.com)
This is the relax-commits mailing list
relax-commits@xxxxxxx
To unsubscribe from this list, get a password
reminder, or change your subscription options,
visit the list information page at
https://mail.gna.org/listinfo/relax-commits
Re: r26197 - /trunk/specific_analyses/relax_disp/api.py