Crystalfield bugfixes #21604

mducle · 2018-01-21T21:18:29Z

This is a collection of bugfixes for the CrystalField calculations routines since the back-end was rewritten last autumn. Due to the changes in the back-end some workflows (which don't have system tests) became broken, and this was not picked up in the unit tests.

The major changes are in the handling of peak widths (setting a resolution model disables peak with fitting and invalidates the .FWHM property; setting .FWHM invalidates a resolution model; previously the behaviour when both are set was ambiguous and in some cases wrong (e.g. fixed FWHM at starting value given in .FWHM)), and in the multi-site routines (particularly handling of background).

In addition, due to requests from users, two additional features were added:

The ability to calculate the dipole transition matrix
The ability to add a residual (background) susceptibility (chi0) to the susceptibility calculations.

The first feature is purely Python, the second involved some C++ changes.

Most of the bugs were in the Python interface (e.g. the arbitrary J calculations was mistakenly removed in the Python interface but the C++ remained), but there were some C++ changes (particularly handling the peak widths) needed.

To test:

Code review and check automated tests run ok.

Test scripts and related datafiles are in the following zip:
cf_test_scripts.zip
Unzip and add the data folder to the Mantid data path and reduction_scripts to the Mantid scripts path and run.

A new system test based on the cf_doc.py file has been added to check that the python syntax works (it runs some fits but the results are not checked - the unit tests are used for checked the numerical validity of the computations).

Fixes #20990

Release Notes

Release notes included.

Reviewer

Please comment on the following (full description):

Code Review

Is the code of an acceptable quality?
Does the code conform to the coding standards?
Are the unit tests small and test the class in isolation?
If there are changes in the release notes then do they describe the changes appropriately?

Functional Tests

Do changes function as described? Add comments below that describe the tests performed?
Do the changes handle unexpected situations, e.g. bad input?
Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

mantid-roman

CrystalFieldControl has field std::vector<int> m_physprops which I think is never used. Can it be deleted?

mantid-roman · 2018-02-01T09:20:31Z

Framework/CurveFitting/src/Functions/CrystalFieldControl.cpp

+      if (m_FWHMs.size() == 1 && m_FWHMs.size() != nSpec) {
+        m_FWHMs.assign(nSpec, m_FWHMs.front());
+      }
+      if (attr.asVector().size() > 0) {


Could you use empty()? attr.asVector() creates a new copy of the vector. Can you use m_FWHMs instead?

mantid-roman · 2018-02-01T09:22:54Z

Framework/CurveFitting/src/Functions/CrystalFieldControl.cpp

      }
+    } else if ((name.compare(0, 5, "FWHMX") == 0 ||
+                name.compare(0, 5, "FWHMY") == 0) &&
+               attr.asVector().size() > 0) {


The same as above

@mantid-roman I can't find a way to avoid using asVector in this case. attr.isEmpty() implicitly assumes the attribute is a string, and so raises an exception in this case (where the attribute is a vector), and there's no other public method to access the contents of the attribute except to using the as* methods...

Could you then use !attr.asVector().empty() ?

Yes. I'll do that.

Also removed dirty_spectra because users can change peak/bg parameters without notifying the CrystalField object, so spectra should always be recalculated.

mantid-roman

Just a couple of minor issues.

mantid-roman · 2018-02-22T08:48:14Z

Framework/CurveFitting/inc/MantidCurveFitting/Functions/CrystalFieldControl.h

@@ -91,7 +91,7 @@ class MANTID_CURVEFITTING_DLL CrystalFieldControl
  /// Cache number of fitted peaks
  // mutable std::vector<size_t> m_nPeaks;
  /// Cache the list of "spectra" corresponding to physical properties
-  std::vector<int> m_physprops;
+  // std::vector<int> m_physprops;


Could you delete this line please? And the m_nPeaks definition as well.

mantid-roman · 2018-02-22T08:50:57Z

Framework/CurveFitting/src/Functions/CrystalFieldSusceptibility.cpp

@@ -139,6 +139,18 @@ void CrystalFieldSusceptibilityBase::function1D(double *out,
  } else {
    calculate(out, xValues, nData, m_en, m_wf, m_nre, H, convfact);
  }
+  const double lambda = getParameter("Lambda");
+  if (fabs(lambda) > 1.e-6) {


Could you define a named constant (all upper case) for 1.e-6?

mantid-roman · 2018-02-22T08:52:07Z

docs/source/fitfunctions/CrystalFieldSusceptibility.rst

@@ -27,18 +27,19 @@ where :math:`N_A` is Avogadro's constant, :math:`k_B` is Boltzmann's constant, :
 crystal field Hamiltonian. :math:`g_J` is the Landé g-factor, :math:`\mu_B` is the Bohr magneton and the moment operator 
 is defined as :math:`\mathbf{J} = \hat{J}_x B_x + \hat{J}_y B_y + \hat{J}_z B_z` where :math:`\hat{J}_x`, :math:`\hat{J}_y`,
 and :math:`\hat{J}_z` are the angular momentum operators in Cartesian coordinates, with :math:`z` defined to 
-be along the quantisation axis of the crystal field (which is usually defined to be the highest symmetry rotation axis). 
+be along the quantisation axis of the crystal fied (which is usually defined to be the highest symmetry rotation axis). 


mantid-roman · 2018-02-22T09:10:45Z

scripts/Inelastic/CrystalField/fitting.py

@@ -261,6 +314,10 @@ def makePhysicalPropertiesFunction(self, i=0):
        if len(fieldParams) > 0:
            out += ',%s' % ','.join(['%s=%s' % item for item in fieldParams.items()])
        ties = self._getFieldTies()
+        if ppobj.TypeID == 2:


Using numbers as type ids is no ideal. Can they be replaced by named constants?

rosswhitfield · 2018-02-23T16:22:33Z

The CrystalFieldPythonInterface system tests fails with python 3 because we don't have PyCifRW for python 3. @martyngigg can we update the Mantid ppa to include a python 3 build and we probably should bump the version to the newest 4.4?

martyngigg · 2018-02-26T08:12:15Z

@rosswhitfield Sure. I'm in the deb packaging zone with bibtex anyway!

martyngigg · 2018-02-26T11:04:12Z

@rosswhitfield The updated version is in the PPA and has the Python 3 packages too.

mducle added 13 commits October 21, 2017 19:08

Re #20990 - Various small bugfixes for Crystal field python code

33beec1

Merge branch 'master' into 20990_crystalfield_bugfixes

60cc0ef

Re #20990 - syntax changes

642b7e8

Re #20990 - made FWHM & Res.Model mutual excl. Fix typos in CFControl.

2dedf2c

Re #20990 - Re-added python code for arbitrary spins J.

5d80376

Re #20990 - Added method to compute dipole transition matrix elements

74f1ab9

Re #20990 - Missing namespace

6a9025e

Re #20990 - fixed bug in multispectrum susceptibility fit

803207c

Re #20990 - added chi0 to crystal field susceptibility

f95ce40

Re #20990 - fixed unit tests and clang-format

3cc231e

Re #20990 - fixed multisite background

f01bc76

Merge branch 'master' into 20990_crystalfield_bugfixes

9ac9ca7

Re #20990 - additional multisite bkg fix + flake8

3896d2e

mducle added the Direct Inelastic Issues and pull requests related to direct inelastic label Jan 21, 2018

mducle requested a review from mantid-roman January 21, 2018 21:18

mducle added 4 commits January 21, 2018 21:24

Re #20990 - clang-format

f624917

Re #20990 - flake8 fix

e686faa

Re #20990 - flake8 fix

2032662

Re #20990 - flake8 fix

a7af0a8

mantid-roman suggested changes Feb 1, 2018

View reviewed changes

mducle added 10 commits February 2, 2018 10:47

Re #20990 - removed unused field, avoid copying attr

603ad65

Re #20990 - changed an isinstance to duck-type.

d32b5d9

Re #20990 - updated docs.

65b2931

Also removed dirty_spectra because users can change peak/bg parameters without notifying the CrystalField object, so spectra should always be recalculated.

Re #20990 - Updated docs. Fixed doc examples python code.

d083fe0

Re #20990 - clang-format. Final doc checks. duck typing.

8fbf0a7

Re #20990 - Added a system test based on the docs for syntax checking.

00026a6

Merge branch 'master' into 20990_crystalfield_bugfixes

1abdca9

Re #20990 - release notes update, flake8

0c178eb

Re #20990 - flake8 fix

81189a1

Re #20990 - sphinx fix

d961e81

mantid-roman suggested changes Feb 22, 2018

View reviewed changes

mducle added 2 commits February 22, 2018 09:45

Re #20990 - code review changes

f1d4f77

Re #20990 - flake8 fix

75394ff

mantid-roman approved these changes Feb 22, 2018

View reviewed changes

martyngigg merged commit 0728c32 into master Feb 22, 2018

martyngigg deleted the 20990_crystalfield_bugfixes branch February 22, 2018 16:27

mducle mentioned this pull request Feb 2, 2022

Crystal fields feature request: calculation of dipole transition matrix #33431

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crystalfield bugfixes #21604

Crystalfield bugfixes #21604

mducle commented Jan 21, 2018 •

edited

Loading

mantid-roman left a comment

mantid-roman Feb 1, 2018

mantid-roman Feb 1, 2018

mducle Feb 6, 2018

mantid-roman Feb 6, 2018 •

edited

Loading

mducle Feb 6, 2018

mantid-roman left a comment

mantid-roman Feb 22, 2018

mantid-roman Feb 22, 2018

mantid-roman Feb 22, 2018

mantid-roman Feb 22, 2018

rosswhitfield commented Feb 23, 2018

martyngigg commented Feb 26, 2018

martyngigg commented Feb 26, 2018

Crystalfield bugfixes #21604

Crystalfield bugfixes #21604

Conversation

mducle commented Jan 21, 2018 • edited Loading

Reviewer

Code Review

Functional Tests

mantid-roman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantid-roman Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mantid-roman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosswhitfield commented Feb 23, 2018

martyngigg commented Feb 26, 2018

martyngigg commented Feb 26, 2018

mducle commented Jan 21, 2018 •

edited

Loading

mantid-roman Feb 6, 2018 •

edited

Loading