Ticket #11056 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

Corrupted state DataObject::Peak

Reported by: Owen Arnold Owned by: Owen Arnold
Priority: critical Milestone: Release 3.4
Component: Framework Keywords:
Cc: saviciat@…, lynchve@… Blocked By: #11004
Blocking: Tester: Martyn Gigg

Description

Doing something like this

The following will leave the peak in a corrupted state for several reasons.

peak = pw.createPeak(qlab, distance)
  • The distance to the detector is required and HKL is not calculated
  • setHKL does not recalculate qLab, qSample, or determine the detector location

Change History

comment:1 Changed 6 years ago by Owen Arnold

  • setQLab should use findDetector. When qLab is set, if we have a goniometer and UB, we should calculate HKL
  • setHKL should determine qLab and then set it via setQLab although, see next point
  • Note that qLab and qSample or not explicitly stored, they are dynamically calculated from the wavlength, the sample and detector location

comment:2 Changed 6 years ago by Owen Arnold

refs #11056. Add regression test.

Prove that this doesn't work yet.

Changeset: a6ab43c4e96f6fa160dd68b9ad42ba1d9fb32c49

comment:3 Changed 6 years ago by Owen Arnold

refs #11056. Peak doesn't assume reference frame.

This was a serious problem that caused a crash with my regression test as soon as I started using my new fake instrument, which is defined in a different reference frame. The setQLab, now uses the instrument reference frame to complete the calculations rather than assuming what it is.

Changeset: b0d5c230ad5bd4a6c25b660fd46d4de7997ea115

comment:4 Changed 6 years ago by Owen Arnold

refs #11056. Peak looks up detector after setQLab.

Refactor to use the ray tracing tools to look up the detector. Distance is optional, but I'd rather remove it altogether.

Changeset: 7db86202b7dca91e192fe9e95274837ea1f56c2a

comment:5 Changed 6 years ago by Owen Arnold

refs #11056. Fix exposing peak to python.

Keep changes to setQLabFrame as expoesed to python instep with what's changed on the c++ side and test it.

Changeset: 941ce1532657ebbd2ec37dc35c01878c5288e5e5

comment:6 Changed 6 years ago by Owen Arnold

refs #11056. New method createPeakHKL.

Unit tested to ensure we are creating a self-consistent peak. Expose to python and unit test there too.

Changeset: 1f5c8337cb1986a47f3cd68bc60c5243a9d3fe70

comment:7 Changed 6 years ago by Owen Arnold

refs #11056. Setup for new AddPeaksHKL algorithm

Changeset: 0d10ba2d78dc6f88df3bc71a4dcd88c03c64fb8a

comment:8 Changed 6 years ago by Owen Arnold

refs #11056. Test and implement.

Changeset: a3b71fed103fdcdcee908649daeb04252dc09123

comment:9 Changed 6 years ago by Owen Arnold

refs #11056. Usage + documentation.

Changeset: 4812d3ae946e2fa9190fc0d9b9dc41c19786c5da

comment:10 Changed 6 years ago by Owen Arnold

  • Status changed from new to verify
  • Resolution set to fixed

This is being verified as pull request #220.

comment:11 Changed 6 years ago by Owen Arnold

Tester:

Run through the usage example against the new algorithm AddPeakHKL (the build server should have done this for you anyway), which touches all the things I have fixed and added as part of this work. Apart from that look at the code. I've provided a number of additional regression tests on the c++ side, and extra tests where I have changed the signature of things exposed to python.

Please check that the SystemTests are still passing before merging into master.

comment:12 Changed 6 years ago by Owen Arnold

refs #11056. No default OL in test creation.

Also fix segfault on PeaksViewer menu option, when no peaks are present, requesting size of the peak information.

Changeset: 2dbf18db0e00a25139dcda7cfb8647aaa9a6aed4

comment:13 Changed 6 years ago by Martyn Gigg

There seem to be quite a few unit test failures

comment:14 Changed 6 years ago by Owen Arnold

refs #11056. Tracked down CrystalTest issues

Changeset: 717bb9cf53f937d6bd54c9c7a84518c728250c62

comment:15 Changed 6 years ago by Owen Arnold

refs #11056. Tracked down MDAlgorithmsTest issues

Changeset: 6f4d2f1aabd475faa2b4602ddfd4204a81bb6cab

comment:16 Changed 6 years ago by Owen Arnold

refs #11056. Fix issue with python exports.

Changeset: 50140bf89c0d79305a62d1af358f7a0f9520fc96

comment:17 Changed 6 years ago by Owen Arnold

refs #11056. Fix GCC warnings.

Changeset: d89e750da68b69a75a99d07de7a5841df3ccd70b

comment:18 Changed 6 years ago by Owen Arnold

Jenkins retest this please

comment:19 Changed 6 years ago by Owen Arnold

refs #11056. Fix IPeak::setQLabFrame defaults issue.

Changeset: b88d144d88310870799e192e038902ccd8a3c6e3

comment:20 Changed 6 years ago by Owen Arnold

Jenkins retest this please.

comment:21 Changed 6 years ago by Owen Arnold

Jenkins retest this please

comment:22 Changed 6 years ago by Owen Arnold

Jenkins retest this please. Internal compiler error for icpc that I can't reproduce on last pull-request build.

comment:23 Changed 6 years ago by Owen Arnold

refs #11056. Fix doxygen warning.

Changeset: c7c5e1b483e581704ce280a89b0834bc853e92d9

comment:24 Changed 6 years ago by Owen Arnold

refs #11056. CreateSampleWorkspace fixed.

CreateSampleWorkspace does not set up a ReferenceFrame which is consistent with the actual layout of the instrument. I don't know how this probelem got in there in the first-place, or why it hadn't cause problem up to now! Peak calculations require that the reference frame is known so that we can dot product with the beam direction in order to determine dQ.

Fixing this should now make the doc-tests pass.

Changeset: 1ec1f8336b0b8017acc4544bcac44b058ae00686

comment:25 Changed 6 years ago by Owen Arnold

Tester. System tests have now been checked against the develop branch.

comment:26 Changed 6 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Tester set to Martyn Gigg

comment:27 Changed 6 years ago by Owen Arnold

refs #11056. Run alg as child.

Changeset: f42f48de1ced2a4dfe573d06f9349cfd4444de26

comment:28 Changed 6 years ago by Martyn Gigg

The code changes look good now and the system tests look fine.

comment:29 Changed 6 years ago by Martyn Gigg

  • Status changed from verifying to closed

Merge pull request #220 from mantidproject/feature/11056_create_peak_hkl

Corrupted state DataObject::Peak

Full changeset: a851158ba0b2f6e4f94d3074d5ce86833de3c46b

comment:30 Changed 6 years ago by Nick Draper

  • Milestone changed from Backlog to Release 3.4

moved to r 3.4 as tickets are closed

comment:31 Changed 6 years ago by Owen Arnold

refs #11056. Create test data.

This is extremely hard to do, which is presumably why this algorithm never had any unit tests in the first place.

Changeset: 47734c5a7d23b57f6e63008c934de802d61e3fd3

comment:32 Changed 6 years ago by Owen Arnold

refs #11056. Upgrade CreateSampleWorkspace

Add options to move the banks and set the pixel spacing for the fake instrument.

This is an important feature of working with SCD data, where we want to move the bank in such a way that we have coverage for specific reflections. I've unit tested the two new properties I've added as part of this. The default behaviour will be exactly the same as before.

Changeset: 17be8bd83ad84e74233ca20f36df4316e9d170aa

comment:33 Changed 6 years ago by Owen Arnold

refs #11056. Get events tests working.

Changeset: 333433e3f59742ec399da5380805e20aee8a84ab

comment:34 Changed 6 years ago by Owen Arnold

refs #11056. Simplify test setup

Also changed the getPeakShape public method to be const, as this is the right sort of behaviour as the PeakShape should be immutable and should not cause internal change to Peak.

Changeset: 5542c3b643a00a01f7fde89bbe57af31a70a343a

comment:35 Changed 6 years ago by Owen Arnold

refs #11056. Check principle axis

I've added some peak by peak checks using the PeakShape to determine what the principle axis used for integration has been. Since I artifically construct the ellipsoids in the test, I know that they have a principle axis directly along the TOF, and there is only one detector involved. We should therefore expect that the principle axis of the ellipsoid is exactly the same as the scattering qDir. Normalising both before comparison takes out the wavenumber dependency.

I've also started adding in the regression tests needed for the Histogram workspace cases.

Changeset: 27759248154995ab10e014c361fab2f53bf2e2c3

comment:36 Changed 6 years ago by Owen Arnold

refs #11056. Working for Workspace2D

My regression tests now pass for both the EventWorkspace case and the Workspace2D case. These are happy path at this point, but they are doing some quite detailed checks and since the test data in both cases are inter-dependent, I know that they are also producing the same output.

There's performance work still yet to do, and I want to probe the behaviour a little deeper, perhaps with some destructive tests thrown in too.

Changeset: 41428ca41d05b2637aa21ab5ebc52f8ffa4eb30c

comment:37 Changed 6 years ago by Owen Arnold

refs #11056. Add destructive and perf tests

Add performance tests prior to optimisation. Add destructive test cases to functional tests.

Changeset: 9189457ae45d09fd9e2357bcd71d9d3121415fa1

comment:38 Changed 5 years ago by Nick Draper

Somehow these slipped through without a resolution. Set to Fixed.

comment:39 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 11895

Note: See TracTickets for help on using tickets.