Ticket #9052 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

IntegrateEllipsoids on all instruments

Reported by: Owen Arnold Owned by: Owen Arnold
Priority: blocker Milestone: Release 3.4
Component: Diffraction Keywords:
Cc: Blocked By:
Blocking: #11307 Tester: VickieLynch

Description (last modified by Nick Draper) (diff)

IntegrateEllipsoids should work on all instruments.

Pascal has reiterated that this is very important to get sorted during this development cycle. This could be down to this ongoing rectangular detector issue.

Change History

comment:1 Changed 7 years ago by Owen Arnold

  • Milestone changed from Backlog to Release 3.2

comment:2 Changed 7 years ago by Nick Draper

  • Owner changed from Anyone to Peter Peterson
  • Status changed from new to infoneeded
  • Component changed from Framework to Diffraction
  • Description modified (diff)
  • Blocking 8930 added

comment:3 Changed 7 years ago by Nick Draper

  • Owner changed from Peter Peterson to Owen Arnold

Can you add a sample case where this is not working?

comment:4 Changed 7 years ago by Owen Arnold

Doesn't work because WISH is collecting Histogram data due to volume. Does not match input restriction for EventWorkspace.

comment:5 Changed 7 years ago by Owen Arnold

  • Status changed from infoneeded to new

comment:6 Changed 7 years ago by Nick Draper

  • Status changed from new to assigned
  • Owner changed from Owen Arnold to Peter Peterson

Pete, feel free to assign onward in your team.

comment:7 Changed 6 years ago by Peter Peterson

  • Milestone changed from Release 3.2 to Release 3.3

comment:8 Changed 6 years ago by Vickie Lynch

  • Blocking 8930 removed

comment:9 Changed 6 years ago by Peter Peterson

  • Milestone changed from Release 3.3 to Release 3.4

I cannot get this done before the release.

comment:10 Changed 6 years ago by Owen Arnold

  • Owner changed from Peter Peterson to Owen Arnold

comment:11 Changed 6 years ago by Owen Arnold

  • Status changed from assigned to inprogress

refs #9052. 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: 5b1edf516271da8c7c4885bbc343d301b3a4887e

comment:12 Changed 6 years ago by Owen Arnold

refs #9052. 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: c08541f30af7839312cb7176eedc9285ed9ee9c5

comment:13 Changed 6 years ago by Owen Arnold

refs #9052. Get events tests working.

Changeset: 4da84e4f26eeeb9da66d5017bcdbabef4f4fe98e

comment:14 Changed 6 years ago by Owen Arnold

refs #9052. 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: 2ad2513054e33da8b2cccf690145ad20cf1e85de

comment:15 Changed 6 years ago by Owen Arnold

refs #9052. 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: 4c514006f4275bb9684087fe32a39ca30aa743de

comment:16 Changed 6 years ago by Owen Arnold

refs #9052. 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.

Conflicts:

Code/Mantid/Framework/MDEvents/src/IntegrateEllipsoids.cpp

Changeset: 342798a091072b9e954580b00d0fde9ce9fc0c95

comment:17 Changed 6 years ago by Owen Arnold

refs #9052. Add destructive and perf tests

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

Changeset: 1fed34dd46bfd93e6930d27e505bb9dd760ee1d6

comment:18 Changed 6 years ago by Owen Arnold

refs #9052. Fix from conflict.

Also update some of the broken documentation.

Changeset: cf6689f2366105d16ced618583adca0736e56925

comment:19 Changed 6 years ago by Owen Arnold

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

This is being verified as ​pull request #317.

comment:20 Changed 6 years ago by Owen Arnold

refs #9052. Fix typdef ambiguity.

Changeset: 42716deb217a252d8a338f942d5d997f6f62682f

comment:21 Changed 6 years ago by Owen Arnold

jenkins test this please

comment:22 Changed 6 years ago by Owen Arnold

refs #9052. Fix accessibility issue.

Changeset: 8329ab7a727a67c505ec768ec4add90ad3a45393

comment:23 Changed 6 years ago by Owen Arnold

Jenkins retest this please

comment:24 Changed 6 years ago by Owen Arnold

Jenkins retest this please

comment:25 Changed 6 years ago by Owen Arnold

Jenkins retest this please

comment:26 Changed 6 years ago by Owen Arnold

refs #9052. Merge in master.

System test changes and others to include in feature branch.

Merge branch 'master' into feature/9052_integrate_ellipsoids

Changeset: 6095d7d60e4fad8187e9489e4d5395f4872be285

comment:27 Changed 6 years ago by Owen Arnold

jenkins retest this please

comment:28 Changed 6 years ago by Owen Arnold

jenkins retest this please

comment:29 Changed 6 years ago by Owen Arnold

Tester, look at the failed test. It's just the windows build where one of the doc tests has spuriously failed.

comment:30 Changed 6 years ago by anonymous

  • Status changed from verify to verifying

comment:31 Changed 6 years ago by VickieLynch

  • Status changed from verifying to closed
  • Tester set to VickieLynch

Merge pull request #317 from mantidproject/feature/9052_integrate_ellipsoids

Make IntegrateEllipsoids universal

Full changeset: ​5af7ec03debfc1a40cc03cf527e400d2d3dcef16

comment:32 Changed 6 years ago by Owen Arnold

  • Blocking 11307 added

comment:33 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue ​9895

Note: See TracTickets for help on using tickets.