Re: Regarding systemtest: ./relax -s Relax_disp.test_bug_21344_sparse_time_spinlock_acquired_r1rho_fail_relax_disp -- February 27, 2014

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.dauvergne@xxxxxxxxx> 
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 <tlinnet@xxxxxxxxxxxxx> 
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.dauvergne@xxxxxxxxx>:

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.dauvergne@xxxxxxxxx>
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 <tlinnet@xxxxxxxxxxxxx>
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

Re: Regarding systemtest: ./relax -s Relax_disp.test_bug_21344_sparse_time_spinlock_acquired_r1rho_fail_relax_disp

Header

Content

Related Messages