1. OpenREM Avatar OpenREM
  2. OpenREM
  3. OpenREM
  4. Issues

Write tests for openSkin

Issue #811 resolved
Ed McDonagh created an issue

openSkin needs a fair amount of refactoring (see Codacy for skinMap.py for example), which would be much easier and safer to do if we have some tests covering the code. This will also make the changes @Luuk and Wens Kong and others wish to make much easier to bring in (see ref #810)

We also need to consider if we bring openSkin into line with PEP8 etc before or after bringing in the changes that will have been written against the current code-base.

Comments (20)

  11. Ed McDonagh reporter

    @Luuk - I have created a branch with the most basic of tests for openSkin, ie the skin map generated is exactly as it is today when generated against the single Siemens RF RDSR currently in the test_files. In order to proceed any further, we need to consider the following:

    • The test is in v1.0 development, i.e. Python 3, Django 2.2. I had to make minor changes to the import statements to get it to run. I need to check the created skin map against the same thing created with v0.10/Python 2.7/Django 1.8
    • I presume all the development work you and Wens have been doing has been against v0.10/Python 2.7/Django 1.8
    • I don’t want to take in any new features against v0.10, all new things should be for v1.0 - what does this mean for your work?
    • I also want to do some basic refactoring of the openSkin code for PEP8 conformance - names of functions and variables etc for example. What does this mean for your work? I am hesitant to write any more tests on a unit level because of wanting to change the names of things!

    I think it would be useful to see what you have done so we can get a better idea of how to pull it all together, even though it is against the old codebase.

    Do you have a Python 3/current develop branch dev setup?

    ‌

  12. Luuk

    Hi Ed,

    • I’m running Python 3.6 now (Wens was already running on 3.6). Change needed to get skinmap running is (for documentation purposes here):

      • geomfunc.py: “from geomclass import *” → “from .geomclass import *”

    • So the development work was against Python 3.6 and Django 2.2

    What do you think is more convenient: first refactoring code for PEP8 conformance or first add our changes? I think for us it doesn’t matter that much.

  13. Ed McDonagh reporter

    That’s great news that Wens was already using Python 3, Django 2.2.

    I can’t comment on what is more convenient, but if refactoring for PEP8 first is ok, then I would very much prefer it. This will enable us to start writing tests for smaller chunks of the code, which will then make adding your changes safer 🙂

    Is refactoring something that you are able to have a go at (or Wens?)

    I will do a PR to merge the existing 811 branch so that the import correction is made (5f2bf80bea76) and the overall test is in the main code, then we can have a new issue for PEP8 refactoring (on a new branch), and we can come back to this issue again later. Does that sound ok?

  18. David Platten

    Added a pickle file to the test results containing skin map data for the RF-RDSR-Siemens-Zee.dcm study calculated using the latest openSkin code. Modified make_skin_map.py so it can be used to return skin dose map data to the calling routine so that can be used in test files. Updated the openskin test file to compare the reference skin dose map data in the pickle file with that calculated from the study. Also updated JavaScript to ensure only one decimal place displayed for phantom height, width and depth. I will carry out a Monte Carlo simulation of this study so we have an independent reference for the peak skin dose (although this will not include the phantom head). Refs issue #811, #933 and #948.

    → <<cset 2046efaf7feb>>

  19. David Platten

    Added png file showing results of Monte Carlo simulation of the RF-RDSR-Siemens-Zee.dcm study. Simulation carried out using 3.5 mm inherent filtration, 8 degree anode angle. The peak skin dose is 1.54 mGy; the peak skin dose from the openSkin calculations is 1.7 mGy, approximately 10 % higher. Refs issue #811

    → <<cset 2708b2447b75>>

  21. Log in to comment
Assignee
–
Type
task
Priority
major
Status
resolved
Component
openSkin
Milestone
1.0.0
Votes
0
Watchers
2
Jira: the preferred issue tracker for Bitbucket. Join the team!