Ticket #11336 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

Clarity around SofQW, SofQW2, SofQW3 confusion

Reported by: Owen Arnold Owned by: Martyn Gigg
Priority: major Milestone: Release 3.4
Component: Framework Keywords: VATES-cli
Cc: nick.draper@… Blocked By:
Blocking: Tester: Owen Arnold

Description

SofQW algorithms have caused confusion for the inelastic group. Ross in particular.

The actions are:

  • Have a conversation with Ross specifically about this.
  • Deprecate algorithms that shouldn't be used, or that aren't right
  • Make the documentation as clear as possible.
  • I noticed in Alex's tutorials that most people loaded up SofQW even though they where asked to use SofQW3, so there may be an issue with naming and usability here.

I've assigned this to Martyn as I know he's worked on this stuff before, but he should reassign if he's not the right person to do this.

Change History

comment:1 Changed 6 years ago by Owen Arnold

I estimate 1/2 man day, unless this involves code changes.

comment:2 Changed 6 years ago by Owen Arnold

  • Cc nick.draper@… added

comment:3 Changed 6 years ago by Nick Draper

Consider leaving SofQW2, and 3 as algorithms, but just as facades to the code moved to real versions of SofQW

comment:4 Changed 6 years ago by Nick Draper

  • Keywords VATES-cli added; VATES removed

comment:5 Changed 6 years ago by Martyn Gigg

My plan for this is to rename:

  • SofQW -> SofQWCentre
  • SofQW2 -> SofQWPolygon (having an alias for SofQW2)
  • SofQW3 -> SofQWPolygonAreaTracking (having an alias for SofQW3)

We can then have SofQW take an additional list parameter for how to do the rebinning.

comment:6 Changed 6 years ago by Martyn Gigg

After discussion with Nick we'll go with

  • SofQW3 -> SofQWNormalisedPolygonArea (having an alias for SofQW3)

and SofQWCentre will be the default so as not to break existing things.

comment:7 Changed 6 years ago by Martyn Gigg

Also need to update the SofQW::summary string to say something about the output format explicitly.

comment:8 Changed 5 years ago by Martyn Gigg

  • Status changed from new to inprogress

Rename algorithms and introduce aliases for the old names

Refs #11336

Changeset: 6d95fb4e5a49a08ba88337af0e301696289900dc

comment:9 Changed 5 years ago by Martyn Gigg

Update documentation after name changes.

Refs #11336

Changeset: 77725d51a74b3ea1b93162436bb61a8094e84dbf

comment:10 Changed 5 years ago by Martyn Gigg

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

This is being verified as pull request #564.

comment:11 Changed 5 years ago by Martyn Gigg

Gut the SofQW algorithm and have it call the appropriate child.

Refs #11336

Changeset: 799cf01e14bce5627b47a12f9a6c5be2eeb12bcf

comment:12 Changed 5 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:13 Changed 5 years ago by Owen Arnold

Changes look good, the documentation is much better. But.

  • SofQW now looks like it ought to be a DataProcessing algorithm rather than just an Algorithm. That way we can extract the child algorithm from the history
  • I would also suggest that SofQWTest include some kind of check that the "Method" parameter resolves to the correct child algorithm. That seemed to be absent.

comment:14 Changed 5 years ago by Martyn Gigg

I debated whether to make it a workflow algorithm or not and as it was only running a single algorithm I wasn't sure it was worth it but it won't do any harm so I'll make the change.

Good point about the unit test, I'll get that updated.

comment:15 Changed 5 years ago by Martyn Gigg

Promote SofQW to a DataProcessorAlgorithm

Refs #11336

Changeset: 61bc449af72508acc790723a8f251d581e88c97d

comment:16 Changed 5 years ago by Martyn Gigg

Clean up the SofQW tests to reduce copy and paste

Refs #11336

Changeset: b5f47a1c1997f84e73d6b81c2b9e7e835209b6df

comment:17 Changed 5 years ago by Martyn Gigg

Add a test for the Method parameter.

Refs #11336

Changeset: 663b3816d07b7c032cd6db67eada019062852765

comment:18 Changed 5 years ago by Martyn Gigg

I've tidied those things up now along with reducing a bit of copy-and-paste that was in the tests.

comment:19 Changed 5 years ago by Martyn Gigg

Strange, that test passed for me when I ran it. I'll take a look.

comment:20 Changed 5 years ago by Martyn Gigg

Fix test for algorithm history.

Refs #11336

Changeset: b5678a6adfa5d2757f6b3749e81a5851a085b378

comment:21 Changed 5 years ago by Martyn Gigg

I can only surmise that I hadn't actually compiled the test properly when I changed the code as it failed this time when I tried it. I've just pushed a fix.

comment:22 Changed 5 years ago by Garrett Granroth

Would it not be better to make a single sofqw algorithm that then has an additional parameter that can be one of �Centre, Normalized Polygon, Or Polygon� It should be up for debate which of the first two its the default.

Best,

Garrett

On Apr 13, 2015, at 9:37 AM, Martyn Gigg <notifications@…<mailto:notifications@…>> wrote:

I can only surmise that I hadn't actually compiled the test properly when I changed the code as it failed this time when I tried it. I've just pushed a fix.

� Reply to this email directly or view it on GitHub<https://github.com/mantidproject/mantid/pull/564#issuecomment-92354274>.

comment:23 Changed 5 years ago by Martyn Gigg

@granrothge That's one of the changes that has gone in here. SofQW is now just a wrapper algorithm that has a Method parameter controlling which of the others is chosen. If this were brand new I would probably pick Polygon as the default but users would have got Centre before so I've kept it as that

comment:24 Changed 5 years ago by Owen Arnold

  • Status changed from verifying to closed

Merge pull request #564 from mantidproject/11336_clean_up_sofqw_naming

Clean up naming of SofQW algoritithms

Full changeset: 25daa7a25415d3547bb61af70252462d4a78cbe5

comment:25 Changed 5 years ago by Nick Draper

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

comment:26 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 12175

Note: See TracTickets for help on using tickets.