Hi,
I'm looking at your patches and here are some comments. Unfortunately
every patch needs either some refinement, or the concept needs to be
changed:
0001-Added-the-write-out-of-dw-and-k_AB-for-model-TSMFK01.patch - Not
applied as the MODEL_TSMFK01 variable is duplicated in one place.
This is an easy fix.
0002-Added-the-spin.model-when-writing-out-parameters.patch - Not
applied as the correct way would be to send the models in as the
'data' argument. This is a generic function for any type of data
associated with a spin system. This can be any NMR parameter, data,
or spin associated concept you can imagine. Therefore adding
arguments for different bits of information, such as model
information, is not how to keep the function general. Such spin
associated information is what the 'data' argument is for. This patch
is the easiest way to have this information sent to a file, but it is
a very undesirable solution. If needed, this should probably be
discussed in a new thread.
0003-Re-inserted-the-writeout-of-the-r2-and-r2a-parameter.patch - Not
applied as this is currently fatal. This can be seen by running the
Relax_disp.test_hansen_cpmg_data_auto_analysis system test. It would
be best to add the infrastructure to support this first. The lines
can be uncommented to aid development, but it should not be committed
yet. I have commented the lines out exactly because they are
currently fatal.
0004-Added-to-calculate-the-tau_cpmg-times-when-model-is-.patch - Not
applied due to code repetition, the same self.tau_cpmg is generated a
few lines below. Code repetition should always be avoided as this
results in a debugging nightmare. The two instances of self.tau_cpmg
being set up might be separated in the future (this will happen if the
__init__() method becomes too long, for example). And the code might
change, as code always does, but only in one location. This is
dangerous! I would suggest having only one "if model in [...]"
statement for the self.tau_cpmg data structure, i.e. separate it out
from the self.power structure. Therefore there would be one if block
for tau_cpmg, and one if block for the matrix exponential power data
structure - each being generated for a different set of models.
0005-Optimized-the-target-function-for-model-TSMFK.patch - Not applied
as there can be slight improvements. The line tau_CP = tcp[i] is not
needed, simply use tcp[i] when it is needed. This Python operation
will cause memory to be allocated to the tau_CP variable, the value
from the numpy data structure copied to the new memory location, and
then that memory destroyed in the next iteration of the loop or when
the function is exited. There's no need to slow the optimisation for
this. Such operations might exist in other modules of lib.dispersion,
but they should all be eliminated. Another point about the patch is
that the indentation for the epydoc arguments in the docstring does
not line up.
0006-Added-the-conversion-to-k_AB-from-kex-and-pA.-k_AB-k.patch - Also
not applied :S This commit is very close though. For the mathematics
operations, please replace '1' with '1.0'. This avoids Python type
conversions (from int to float).
Cheers,
Edward
On 10 September 2013 12:29, Troels E. Linnet
<NO-REPLY.INVALID-ADDRESS@xxxxxxx> wrote:
Follow-up Comment #69, sr #3071 (project relax):
Modified write out functions for parameters and optimized the target
function.
(file #18955)
_______________________________________________________
Additional Item Attachment:
File name: write_out_patches.patch.tar.gz Size:5 KB
_______________________________________________________
Reply to this item at:
<http://gna.org/support/?3071>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
relax (http://www.nmr-relax.com)
This is the relax-devel mailing list
relax-devel@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-devel
Re: [sr #3071] Implementation of Tollinger/Kay dispersion model (2001)