Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enumerated string property #38177

Merged
merged 19 commits into from
Oct 24, 2024
Merged

Enumerated string property #38177

merged 19 commits into from
Oct 24, 2024

Conversation

dmitry-ganyushin
Copy link
Contributor

7509: [Story] create EnumeratedStringProperty, new approach

Description of work

Creating EnumeratedStringProperty structure to simplify string properties in Mantid that are restricted to specific values. This new property type will replace most current string properties.

rboston628 and others added 5 commits October 3, 2024 13:48
Refactoring
using EnumeratedStringProperty in CalculateDIFC.cpp
… enumerated-string-property

# Conflicts:
#	Framework/Algorithms/src/AddSampleLog.cpp
@sf1919 sf1919 added this to the Release 6.12 milestone Oct 15, 2024
/// The value of the property
ENUMSTRING m_value;
/// the property's default value which is also its initial value
// const TYPE m_initialValue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this line

Copy link
Contributor Author

@dmitry-ganyushin dmitry-ganyushin Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remove it, I have the following compilation error:

/home/yvg/mantid/Framework/Kernel/inc/MantidKernel/EnumeratedStringProperty.hxx:279:9: error: 'class Mantid::Kernel::EnumeratedStringProperty<{anonymous}::TypeMode, (& {anonymous}::typeOptions)>' has no member named 'm_value'; did you mean 'value'?
  279 |   this->m_value = static_cast<EnumeratedString<E, names>>(value);
      |   ~~~~~~^~~~~~~
      |   value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think he means the commented out line of code

@@ -73,7 +73,7 @@ def test_alg_has_expected_doc_string(self):
)
doc = simpleapi.rebin.__doc__
self.assertGreater(len(doc), 0)
self.assertEqual(doc, expected_doc)
self.assertEqual(doc[0:1897], expected_doc[0:1897])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to only check the first 1897 characters returned?

Copy link
Contributor Author

@dmitry-ganyushin dmitry-ganyushin Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The missing part of the docs string here contains:
BinningMode(Input) *string* ...
It looks like the expected_doc string is generated during build, and instead of *string* contains some machine-generated symbols because of a new object EnumeratedStringProperty. Right now I don't have a better solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment in the code that describes this oddity. Future you will thank current you for being so forward thinking in explaining why in a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment

Copy link
Member

@peterfpeterson peterfpeterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would greatly benefit from an additional section/page in the developer documentation explaining how to use it. It would benefit from linking to some of these algorithm cpp files as examples.

Also, you deleted most of the PR template (example: how to test).

@rboston628
Copy link
Contributor

Pulled, built, tested in workbench with this script:

# import mantid algorithms, numpy and matplotlib
from mantid.simpleapi import *
from mantid.api import AlgorithmManager
import matplotlib.pyplot as plt
import numpy as np
from mantid.testing import assert_almost_equal as assert_wksp_equal

ws = mtd.unique_name(prefix="test_")
dataX = [1, 2, 3, 4, 5, 6]
dataY = [6, 6, 7, 6, 6]
CreateWorkspace(
    OutputWorkspace=ws,
    DataX = dataX,
    DataY = dataY,
    NSpec=1,
)

# check AddSampleLog

strprop = mtd.unique_name(prefix="prop_")
randomtext = mtd.unique_name(64)
AddSampleLog(
    Workspace=ws,
    LogName=strprop,
    LogText=randomtext,
    LogType="String",
)

number = 1.1234

defprop = mtd.unique_name(prefix="prop_")
AddSampleLog(
    Workspace=ws,
    LogName=defprop,
    LogText=str(number),
    LogType="Number",
)

doubleprop = mtd.unique_name(prefix="prop_")
AddSampleLog(
    Workspace=ws,
    LogName=doubleprop,
    LogText=str(number),
    LogType="Number",
    NumberType="Double",
)

run = mtd[ws].run()
assert run.getProperty(strprop).value == randomtext
assert run.getProperty(defprop).value == number
assert run.getProperty(doubleprop).value == number

# check Rebin

Rebin(
    InputWorkspace=ws,
    OutputWorkspace=ws+"_default",
    Params=(1, 2, 6),
)

Rebin(
    InputWorkspace=ws,
    OutputWorkspace=ws+"_linear",
    Params=(1, -2, 6),
    BinningMode="Linear",
)

CreateWorkspace(
    OutputWorkspace="exp_lin",
    DataX = [dataX[0], dataX[2], dataX[4], dataX[-1]],
    DataY = [sum(dataY[i:i+2]) for i in range(0, 6, 2)],
    NSpec=1,
)
assert_wksp_equal(ws+"_default", ws+"_linear")
assert_wksp_equal(ws+"_default", "exp_lin")

Rebin(
    InputWorkspace=ws,
    OutputWorkspace=ws+"_log",
    Params=(1, 2, 6),
    BinningMode="Logarithmic",
)
CreateWorkspace(
    OutputWorkspace="exp_log",
    DataX = [dataX[0], dataX[2], dataX[5]],
    DataY = [sum(dataY[0:2]), sum(dataY[2:])],
    NSpec=1,
)
assert_wksp_equal(ws+"_log", "exp_log")


# check values
from mantid.api import AlgorithmManager
algo = AlgorithmManager.create("Rebin")
algo.initialize()
algo.setProperty("BinningMode", "Linear")
assert "Linear" == algo.getPropertyValue("BinningMode")
assert "Linear" == algo.getProperty("BinningMode").valueAsStr
assert "Linear" == algo.getProperty("BinningMode").value

This checks several of the algorithms changed to use EnumeratedStringProperty and verifies they work as expected. In particular, the value of the enumerated string can be accessed and compared to a string.

@peterfpeterson peterfpeterson merged commit 980aafa into main Oct 24, 2024
10 checks passed
@peterfpeterson peterfpeterson deleted the enumerated-string-property branch October 24, 2024 15:09
@dmitry-ganyushin dmitry-ganyushin restored the enumerated-string-property branch October 24, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants