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 <[email protected]> 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 <[email protected]>
> 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 <[email protected]>:
>>
>>> 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 <[email protected]>
>>> 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 <[email protected]>
>>> > 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
[email protected]
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
