Hi Troels,

I may have made a mistake about the find_intensity_keys() function!  I
was looking at why a number of the system tests now fail, and I
realised that there is a case were multiple IDs exist for the same
{exp_type, frq, offset, point, time} set - replicated spectra!  For
one metadata set, there can be multiple peak intensity values.  I
don't know why I forgot about this.  That is why there was an 's' at
the end of the function name and why it returns a list.  It might be
worth reverting commits {r22357, r22358, r22360}, but leaving {r22359,
r22361, r22362}.  Sorry for the inconvenience.

I've also looked at the return_intensity() function and can see that
its logic is plain wrong.  It returns the first intensity of the list
of replicates returned by find_intensity_keys() (or previously the
'key' variable which did not exist).  This makes no sense.  But is
also not called anywhere within relax.  I guess it has been 100%
replaced by the average_intensity() function.  Instead of having a
unit test for it, maybe it would be best to remove the function.  The
function looks like old, unused code, so removing it is a good idea.

Cheers,

Edward



On 27 February 2014 13:09, Edward d'Auvergne <edward.dauver...@gmail.com> wrote:
> :)  I'm looking through the functions of the disp_data module and have
> found one other place where the offset should be supported - the
> loop_spectrum_ids() function.  As the test suite now passes, it might
> be a good idea to create a quick unit test for each function you
> change.  This is just to be certain that each function is functioning
> as you would expect.  Using your CPMG and R1rho data saved states, the
> function could be tested on both to make sure that all data types
> operate correctly.
>
> Note that in the future, relax many support offset effects in the CPMG
> as well.  This is currently the major benefit of using Flemming
> Hansen's CATIA software.  Such support would be quite easy to add, as
> relax already handles the offset concept for R1rho data and the use of
> R1 data.  It would simply require a new user function for inputting
> hard pulse information and a new numeric model which uses the correct
> matrices.  This information is just for future reference, there is no
> need for an implementation at the moment.  Therefore this is listed in
> the TODO section of the dispersion chapter in the manual
> (http://www.nmr-relax.com/manual/do_dispersion_features_yet_be_implemented.html)
>
> Cheers,
>
> Edward
>
>
> On 27 February 2014 12:40, Troels Emtekær Linnet <tlin...@nmr-relax.com> 
> wrote:
>> That sounds perfect. :-)
>>
>> And I say, that I now know what to do after Lunch.
>>
>> Never gonna give "it" up... never gonna...
>> https://www.youtube.com/watch?v=dQw4w9WgXcQ
>>
>> Best
>> Troels
>>
>>
>>
>>
>> 2014-02-27 12:37 GMT+01:00 Edward d'Auvergne <edward.dauver...@gmail.com>:
>>
>>> Actually, by grepping the source code:
>>>
>>> $ grep "find_intensity_keys([a-z]" -r . --exclude-dir=.svn
>>>
>>> I can see many places where find_intensity_keys() is incorrectly
>>> called!  I think it's obvious that this function came before
>>> R1rho-type data was supported.  Looking further, the
>>> return_intensity() function is also deficient for the offset value!
>>> Almost everywhere where find_intensity_keys() is called is incorrectly
>>> implemented (excluding the specific_analyses.relax_disp.nessy module
>>> as NESSY does not support R1rho data)!  These should all be fixed
>>> otherwise problems will appear in your analysis later on.
>>>
>>> Thinking about the problem even more, I can see that only one key is
>>> ever used when calling find_intensity_keys().  The purpose of having a
>>> list of keys has been permanently lost - I remember implementing this
>>> as a feature earlier in the relax_disp branch development but the
>>> reason for it no longer exists.  As a clean solution, I would suggest:
>>>
>>> - Renaming the function to find_intensity_key() and have it return a
>>> single value.
>>> - Have the find_intensity_key() function raise a RelaxError if more
>>> than one key is found.
>>> - Modify all of the code calling find_intensity_key() to expect and
>>> handle a single key.
>>> - Modify the return_intensity() function to be more like the
>>> average_intensity() function in that the offset argument is supported.
>>>
>>> This solution would be much more maintainable in the future.  What do you
>>> think?
>>>
>>> Regards,
>>>
>>> Edward
>>>
>>> On 27 February 2014 12:08, Edward d'Auvergne <edward.dauver...@gmail.com>
>>> wrote:
>>> > Hi,
>>> >
>>> > This looks like another rather stupid typo/mistake :)  I added a print
>>> > statement for the int_keys variable and ran the test.  There should
>>> > only be one key for a given {exp_type, frq, offset, point, time}
>>> > metadata set, but the printout shows this not to be the case.  If you
>>> > carefully look at the find_intensity_keys() call, you will see that
>>> > one of these 5 bits of information are not sent in as an argument ;)
>>> >
>>> > Regards,
>>> >
>>> > Edward
>>> >
>>> >
>>> >
>>> >
>>> > On 27 February 2014 11:38, Troels Emtekær Linnet <tlin...@nmr-relax.com>
>>> > wrote:
>>> >> Hi Edward.
>>> >>
>>> >> When I run:
>>> >> ./relax -s
>>> >> Relax_disp.test_bug_21344_sparse_time_spinlock_acquired_r1rho_fail_relax_disp
>>> >>
>>> >> I get:
>>> >> -------------------
>>> >> Parameter values: [2.4392597217423719, 149801.17120634759]
>>> >> Function value:   252.36349493927844
>>> >> Iterations:       135
>>> >> Function calls:   281
>>> >> Gradient calls:   0
>>> >> Hessian calls:    0
>>> >> Warning:          None
>>> >>
>>> >>
>>> >> relax> eliminate(function=None, args=None)
>>> >>
>>> >> relax> monte_carlo.setup(number=5)
>>> >>
>>> >> relax> monte_carlo.create_data(method='back_calc')
>>> >> Traceback (most recent call last):
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/test_suite/system_tests/relax_disp.py",
>>> >> line 284, in
>>> >> test_bug_21344_sparse_time_spinlock_acquired_r1rho_fail_relax_disp
>>> >>     relax_disp.Relax_disp(pipe_name='base pipe',
>>> >> pipe_bundle='relax_disp', results_dir=self.tmpdir, models=['R2eff'],
>>> >> grid_inc=3, mc_sim_num=5, modsel='AIC', pre_run_dir=None,
>>> >> insignificance=1.0, numeric_only=False, mc_sim_all_models=False,
>>> >> eliminate=True)
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/auto_analyses/relax_disp.py",
>>> >> line 118, in __init__
>>> >>     self.run()
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/auto_analyses/relax_disp.py",
>>> >> line 471, in run
>>> >>     self.optimise(model=model)
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/auto_analyses/relax_disp.py",
>>> >> line 379, in optimise
>>> >>     self.interpreter.monte_carlo.create_data()
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/prompt/uf_objects.py",
>>> >> line 221, in __call__
>>> >>     self._backend(*new_args, **uf_kargs)
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/pipe_control/monte_carlo.py",
>>> >> line 113, in create_data
>>> >>     pack_sim_data(data_index, random)
>>> >>   File
>>> >> "/sbinlab2/tlinnet/software/NMR-relax/relax_trunk/specific_analyses/relax_disp/api.py",
>>> >> line 1609, in sim_pack_data
>>> >>     raise RelaxError("Monte Carlo simulation data for the key '%s'
>>> >> already exists." % int_key)
>>> >> RelaxError: RelaxError: Monte Carlo simulation data for the key
>>> >> '1_0_46_0' already exists.
>>> >>
>>> >> ---------------------
>>> >>
>>> >> I have looked into:
>>> >> specific_analyses/relax_disp/api.py
>>> >>
>>> >> There it is:
>>> >> ----------------------------------------------
>>> >>     def sim_pack_data(self, data_id, sim_data):
>>> >>         """Pack the Monte Carlo simulation data.
>>> >>
>>> >>         @param data_id:     The tuple of the spin container and the
>>> >> exponential curve identifying key, as yielded by the base_data_loop()
>>> >> generator method.
>>> >>         @type data_id:      SpinContainer instance and float
>>> >>         @param sim_data:    The Monte Carlo simulation data.
>>> >>         @type sim_data:     list of float
>>> >>         """
>>> >>
>>> >>         # The R2eff model (with peak intensity base data).
>>> >>         if cdp.model_type == 'R2eff':
>>> >>             # Unpack the data.
>>> >>             spin, exp_type, frq, offset, point = data_id
>>> >>
>>> >>             # Initialise the data structure if needed.
>>> >>             if not hasattr(spin, 'intensity_sim'):
>>> >>                 spin.intensity_sim = {}
>>> >>
>>> >>             # Loop over each time point.
>>> >>             ti = 0
>>> >>             for time in loop_time(exp_type=exp_type, frq=frq,
>>> >> offset=offset, point=point):
>>> >>                 # Get the intensity keys.
>>> >>                 int_keys = find_intensity_keys(exp_type=exp_type,
>>> >> frq=frq, point=point, time=time)
>>> >>
>>> >>                 # Loop over the intensity keys.
>>> >>                 for int_key in int_keys:
>>> >>                     # Test if the simulation data point already exists.
>>> >>                     if int_key in spin.intensity_sim:
>>> >>                         raise RelaxError("Monte Carlo simulation data
>>> >> for the key '%s' already exists." % int_key)
>>> >>
>>> >>                     # Initialise the list.
>>> >>                     spin.intensity_sim[int_key] = []
>>> >>
>>> >>                     # Loop over the simulations, appending the
>>> >> corresponding data.
>>> >>                     for i in range(cdp.sim_number):
>>> >>
>>> >> spin.intensity_sim[int_key].append(sim_data[i][ti])
>>> >>
>>> >>                 # Increment the time index.
>>> >>                 ti += 1
>>> >> ---------------------
>>> >>
>>> >> For me, this looks okay.
>>> >>
>>> >> Do you have an Idea why this is not working?
>>> >>
>>> >> Best
>>> >> Troels
>>
>>

_______________________________________________
relax (http://www.nmr-relax.com)

This is the relax-devel mailing list
relax-devel@gna.org

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

Reply via email to