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