Ticket #11336 (closed: fixed)
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: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: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
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
I estimate 1/2 man day, unless this involves code changes.