Ticket #10279 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

declare 'Shift' parameter for TabulatedFunction

Reported by: Jose Borreguero Owned by: Jose Borreguero
Priority: major Milestone: Release 3.3
Component: Framework Keywords:
Cc: Blocked By: #10287
Blocking: #10189 Tester: Anders Markvardsen

Description

Indicated for the scenario when we want to compare two workspaces representing two structure factors that may be shifted with respect to each other.

Attachments

q300.nxs (78.8 KB) - added by Jose Borreguero 6 years ago.
QENS dynamics structure factor
script.py (421 bytes) - added by Jose Borreguero 6 years ago.
fit the data

Change History

comment:1 Changed 6 years ago by Jose Borreguero

  • Status changed from new to assigned

comment:2 Changed 6 years ago by Jose Borreguero

  • Status changed from assigned to inprogress

Refs #10279 Added Centre parameter

Changeset: d3891e486f95b1c8390c42eabd79ff484e1df2d1

Changed 6 years ago by Jose Borreguero

QENS dynamics structure factor

Changed 6 years ago by Jose Borreguero

fit the data

comment:3 Changed 6 years ago by Jose Borreguero

Refs #10279 Bypass the test for the moment

Changeset: 1643dc7f4f5bd68025f70d391eb68b8594c84d7c

comment:4 Changed 6 years ago by Jose Borreguero

Refs #10279 Repaired test_derivatives

Changeset: a1bccc219b35f726b8185f8051b5a5d0b39a0a89

comment:5 Changed 6 years ago by Anders Markvardsen

  • Blocked By 10287 added

Hi Jose,

I have put this ticket as blocked by #10287.

The reason is that your changes have caused a ISIS SANS LOQ systemtest to fail. Please read comment 7 of this ticket, it has been found that your changes to TabulatedFunction is unfortunately now causing TabulatedFunction to fail, see comment 7 in #10287.

Could you either update the code referred to in comment 7 of #10287 or advice me on how I can get TabulatedFunction to work again in the code referred to in this comment 7

comment:6 Changed 6 years ago by Jose Borreguero

Refs #10279 Fixed numerical deriv and added usage example

Changeset: 25c74fb856e5d13566e538e63880b92a5d7186f8

comment:7 Changed 6 years ago by Jose Borreguero

Refs #10279 static cast required to avoid compiler warning

Changeset: 41ff6e3a27808c9069ad5e745a7e640ff451c8f6

comment:8 Changed 6 years ago by Jose Borreguero

  • Blocking 10189 added

comment:9 follow-up: ↓ 11 Changed 6 years ago by Martyn Gigg

There is a Sphinx documentation warning in TabulatedFunction.rst: http://builds.mantidproject.org/job/develop_clean/label=win7-build/377/warnings3Result/new/

comment:10 Changed 6 years ago by Anders Markvardsen

  • Blocking 10287 added; 10189 removed
  • Blocked By 10287 removed
  • Summary changed from declare 'centre' parameter for TabulatedFunction to declare 'Shift' parameter for TabulatedFunction

Hi Jose,

I have removed the block.... After a few more hours I eventually found that one additional reason for the failure was that an additional parameter means that the fitting parameter table output contains one additional row, hence any python code that is using TabulatedFunction fit-function and try to retrive information about the fitted parameter will almost certainly now fails.

More specifically code like this will now fail:

{

param = mtd__fitRescaleAndShift_Parameters?

row1 = param.row(0).items() row2 = param.row(1).items() row3 = param.row(2).items() row4 = param.row(3).items() scale = row1[1][1] shift = row3[1][1] chiSquared = row4[1][1]

}

In addition, for example, in the affected code in #10287, after speaking to SANS instrument scientist, they specifically do not want a Shift parameter to be fitting since they are fitting two spectra against each other that have fixed x-values.

Therefore before putting this ticket to fixed please:

Check to see if TabulatedFunction is used anywhere in the Mantid code. By default assume that the behaviour is to keep Shift fixed. Note in the Code/Mantid/script folder it only used once and this is relate to #10287, and I can update this when relevant.

No c++ algorithms appear to use TabulatedFunction, but I haven’t check python algorithms. However Resolution fit-function uses TabulatedFunction, but speaking to Roman this is OK since here TabulatedFunction parameters are not fitted here. TabulatedFunction is used in ConvolveWorkspaces – check that this is OK.

Optionally email Mantid-develop to inform about this change.

Make sure that the next release note mention that TabulatedFunction have been updated with an additional fitting parameter.

Note if you find that TabulatedFunction is used too heavily, consider leaving TabulatedFunction as is and add a function TabulatedFunctionWithShift.

I have reversed the block so that this ticket now blocks #10287

comment:11 in reply to: ↑ 9 Changed 6 years ago by Jose Borreguero

Replying to Martyn Gigg:

There is a Sphinx documentation warning in TabulatedFunction.rst: http://builds.mantidproject.org/job/develop_clean/label=win7-build/377/warnings3Result/new/

Thanks, I had missed this. I'll correct it ASAP

comment:12 Changed 6 years ago by Jose Borreguero

Hi Anders,

Algorithm ConvolveWorkspaces doesn't fit anything so parameters of Tabulatedfunction are of no relevance. Their default values are used.

Resolution does not have fitting parameters, so again, they are of no relevance. By the way, I don't see the point of this Resolution function. I can't make the difference between this function and a TabulatedFunction with fixed fitting parameters. In fact, a resolution function should incorporate a Shift parameter to accommodate deviation of the experimental data from the origin of energy transfers when fitting against a model, but this is an issue for another ticket.

comment:13 Changed 6 years ago by Anders Markvardsen

  • Blocking 10287 removed

comment:14 Changed 6 years ago by Jose Borreguero

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

comment:15 Changed 6 years ago by Jose Borreguero

Note to tester:

Build the documentation too, and test for yourself the example given there.

comment:16 Changed 6 years ago by Anders Markvardsen

  • Status changed from verify to verifying
  • Tester set to Anders Markvardsen

comment:17 Changed 6 years ago by Anders Markvardsen

  • Status changed from verifying to closed

Branched has been merged into master, but this was not recorded on this ticket

comment:18 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 11121

Note: See TracTickets for help on using tickets.