Add unit tests for ISIS_Powder common routines #19163

DavidFair · 2017-03-16T14:59:26Z

Must not be merged before #19112 - I would recommend waiting for that PR before taking this one so you can get an accurate diff

Description of work.
This PR adds unit tests for the common functions in the ISIS_Powder scripts

To test:
Check that unit tests pass
Code review

Fixes #19113 .

Does not need to be in the release notes.

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? Is it well structured with small focussed classes/methods/functions?
Are there unit/system tests in place? Are the unit tests small and test the a 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?
How do the changes handle unexpected situations, e.g. bad input?
Has the relevant documentation been added/updated?
Is user-facing documentation written in a user-friendly manner?
Has developer documentation been updated if required?
Does everything look good? Comment with the ship it emoji but don't merge. A member of @mantidproject/gatekeepers will take care of it.

…EM' into 19113_add_unit_tests_ISIS_Powder_common Re #19113 Merge changes from GEM migration

dtasev · 2017-03-24T09:39:04Z

scripts/Diffraction/isis_powder/routines/InstrumentSettings.py

@@ -65,9 +65,9 @@ def update_attributes(self, advanced_config=None, basic_config=None, kwargs=None
            self._parse_attributes(self._adv_config_dict, suppress_warnings=suppress_warnings)
        if advanced_config or basic_config:
            self._parse_attributes(self._basic_conf_dict,
-                                   suppress_warnings=(not bool(basic_config or suppress_warnings)))
+                                   suppress_warnings=(not bool(basic_config) or suppress_warnings))


More of a query than a note, do we have to cast implicitly to bool? I think it's done automatically when you put the not in front, i.e. there shouldn't be a difference between not bool(basic_config) and just not basic_config

Good point Python does this implicitly I'll change that in the current branch I'm working on (to reduce the number of merge conflict in the future)

bump to remove bool, unit tests passed

dtasev · 2017-03-24T09:52:23Z

From a brief look changes look good, all the unit tests pass, I'll have a look again after #19112 is merged and mark for ship then

dtasev · 2017-04-05T11:35:26Z

scripts/Diffraction/isis_powder/routines/InstrumentSettings.py

        if advanced_config or basic_config or kwargs:
-            self._parse_attributes(self._kwargs, suppress_warnings=(not bool(kwargs or suppress_warnings)))
+            self._parse_attributes(self._kwargs, suppress_warnings=(not bool(kwargs)) or suppress_warnings)


(not bool(kwargs)) or suppress_warnings)
is the 2nd bracket after kwargs a mistake?

dtasev · 2017-04-05T11:35:35Z

scripts/Diffraction/isis_powder/routines/InstrumentSettings.py

@@ -65,9 +65,9 @@ def update_attributes(self, advanced_config=None, basic_config=None, kwargs=None
            self._parse_attributes(self._adv_config_dict, suppress_warnings=suppress_warnings)
        if advanced_config or basic_config:
            self._parse_attributes(self._basic_conf_dict,
-                                   suppress_warnings=(not bool(basic_config or suppress_warnings)))
+                                   suppress_warnings=(not bool(basic_config) or suppress_warnings))


bump to remove bool, unit tests passed

dtasev · 2017-04-05T12:28:15Z

scripts/Diffraction/isis_powder/routines/yaml_parser.py

-                                 "This should be a range of runs in this cycle in the mapping file."
-                                 " Please check your indentation if this should be within a cycle.")
+                                 "This should be a range of runs in the mapping file."
+                                 " Please check your indentation if the syntax looks correct.")


Please check your indentation if the syntax looks correct. consider rephrasing to something like
Please check your indentation and make sure that the syntax is correct.

dtasev · 2017-04-05T12:38:24Z

scripts/Diffraction/isis_powder/routines/common.py

@@ -156,7 +157,10 @@ def get_first_run_number(run_number_string):
    :return: The first run for the user input of runs
    """
    run_numbers = generate_run_numbers(run_number_string=run_number_string)
-    return run_numbers[0]
+    if isinstance(run_numbers, list):


It seems like generate_run_numbers always returns a list, with single run number, or a range

also in get_monitor_ws here, I think you could use get_first_run_number

Thats correct - it means the caller doesn't have to figure out types on their end (e.g. can just use a for in loop or [0])

Good spot 👍

dtasev · 2017-04-05T12:44:14Z

Use get_first_run_number in yaml_parser.py here ?

dtasev · 2017-04-19T12:59:45Z

when OSX build lands on a node which has yaml installed

DavidFair added 19 commits March 14, 2017 10:11

Merge remote-tracking branch 'origin/18643_ISIS_Powder_Migration_Of_G…

ab2b8f1

…EM' into 19113_add_unit_tests_ISIS_Powder_common Re #19113 Merge changes from GEM migration

Re #19113 Add unit test files to CMake list

befb9d0

Re #19113 Added unit test for crop_banks_in_tof

dd0f76f

Re #19113 Added unit test for crop_in_tof

70d36f9

Re #19113 Added test for extract_ws_spectra

3225dee

Re #19113 Added test for generate_run_numbers

26c1a8d

Re #19113 Added test data from POLARIS instrument

69d7b3f

Re #19113 Start adding test for load_current_normalised_ws_list

d929bb3

Re #19113 Added test for load_current_normalised_ws_list

9d6d4af

Re #19113 Added test for spline vanadium ws

0fd7f94

Re #19113 Added tests for updating InstrumentSettings values

7ef9af7

Re #19113 Added test that recursion happened in InstrumentSettings

320dcc8

Re #19113 Added tests for optional params in InstrumentSettings

18b74b6

Re #19113 Test setting value to same value does not emit warning

6058a81

Re #19113 Remove debug lines from Engg algs

64e3c0b

Re #19113 Added test that missing key is detected in Abs_corrections

3e84b6d

Re #19113 Add test when number density is required in Abs_corrections

7e4627a

Re #19113 Added tests for YAML Parser

f559292

Re #19113 Added tests for sanity checking of YAML files

a97be16

DavidFair added Component: Python Powder Issues and pull requests related to powder diffraction labels Mar 16, 2017

DavidFair added this to the Release 3.10 milestone Mar 16, 2017

DavidFair added this to PR in ISIS Powder Diffraction Mar 16, 2017

DavidFair and others added 3 commits March 16, 2017 17:28

Re #19113 Remove unit test that times out

abbb250

Re #19113 Fix compatibility with Python 3

9a5545b

Merge branch 'master' into 19113_add_unit_tests_ISIS_Powder_common

67975ef

DavidFair mentioned this pull request Mar 20, 2017

GEM Intial Feedback fixes #19193

Merged

9 tasks

dtasev self-assigned this Mar 22, 2017

dtasev reviewed Mar 24, 2017

View reviewed changes

DavidFair added 2 commits April 3, 2017 17:00

Merge branch 'master' into 19113_add_unit_tests_ISIS_Powder_common

a86146c

Re #19113 Fixed syntax error following merge

ac05bc1

dtasev suggested changes Apr 5, 2017

View reviewed changes

Re #19113 Added feedback from PR

fdf327f

dtasev approved these changes Apr 7, 2017

View reviewed changes

SimonHeybrock merged commit eb80002 into master Apr 21, 2017

SimonHeybrock deleted the 19113_add_unit_tests_ISIS_Powder_common branch April 21, 2017 12:46

DavidFair moved this from PR to Done in ISIS Powder Diffraction Apr 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for ISIS_Powder common routines #19163

Add unit tests for ISIS_Powder common routines #19163

DavidFair commented Mar 16, 2017 •

edited by dtasev

dtasev Mar 24, 2017

DavidFair Apr 3, 2017

dtasev Apr 5, 2017

dtasev commented Mar 24, 2017

dtasev Apr 5, 2017

dtasev Apr 5, 2017

dtasev Apr 5, 2017

dtasev Apr 5, 2017

dtasev Apr 5, 2017

DavidFair Apr 7, 2017

dtasev commented Apr 5, 2017

dtasev commented Apr 19, 2017 •

edited

Navigation Menu

Add unit tests for ISIS_Powder common routines #19163

Add unit tests for ISIS_Powder common routines #19163

Conversation

DavidFair commented Mar 16, 2017 • edited by dtasev

Reviewer

Code Review

Functional Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtasev commented Mar 24, 2017

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

Choose a reason for hiding this comment

dtasev commented Apr 5, 2017

dtasev commented Apr 19, 2017 • edited

DavidFair commented Mar 16, 2017 •

edited by dtasev

dtasev commented Apr 19, 2017 •

edited