Context Navigation

← Previous Ticket
Next Ticket →

Ticket #8116 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

Refactoring of FlatAbs for a pythonic version

Reported by:	Gesner Passos	Owned by:	Samuel Jackson
Priority:	minor	Milestone:	Release 3.1
Component:	Indirect Inelastic	Keywords:
Cc:		Blocked By:
Blocking:		Tester:

Description

The FlatAbs function in Inelastic/IndirectAbsCor.py was given by Spencer and it seems to mimic his previous fortran code (It was introduced in #7976).

It does not exploit important python tools like:

List Comprehension: http://docs.python.org/2/tutorial/datastructures.html#tut-listcomps
Numpy lib.

And it should also provide information about how it works and why (maybe a reference where this algorithm was first presented).

Finally, it breaks the convention on variable names and our code style (http://www.mantidproject.org/Coding_Standards). And the name of the variables do not help to understand the code.

Change History

comment:1 Changed 7 years ago by Samuel Jackson

Status changed from new to inprogress

Refs #8116 Refactored FlatAbs

Changeset: 4ea2cbdda2bcaa792c98d5ca0bc0f33311b34613

comment:2 Changed 7 years ago by Samuel Jackson

Refs #8116 Added sources for further information on technique.

Changeset: 7abd881004fdfb211ec17b17f7fa06c968c012e6

comment:3 Changed 7 years ago by Samuel Jackson

Status changed from inprogress to verify
Resolution set to fixed

To Tester

I've done the best I can with this. The names are now more like the standards we have throughout the project. I've tried to make them more verbose where I can. I've also added some sources for further information.

Do a code inspection and check the changes are appropriate. The system tests should prove that I haven't broken this technique, but it might be good to run through Apply Corrections and check it still works. Instructions to do this can be found here:

http://www.mantidproject.org/Indirect:Indirect_Data_Analysis#Running_Apply_Corrections

comment:4 Changed 7 years ago by Samuel Jackson

Status changed from verify to reopened
Resolution fixed deleted

comment:5 Changed 7 years ago by Samuel Jackson

Status changed from reopened to inprogress

Refs #8116 Fix for system test.

Changeset: 29258bc6e10531e9668f98fc5d15ffd645e25dd2

comment:6 Changed 7 years ago by Samuel Jackson

Refs #8116 Correction for system test.

Changeset: 48ed2cc8060ac8023b63b11c718ae7e6f71dfa80

comment:7 Changed 7 years ago by Samuel Jackson

Refs #8116 Refactor code into functions

Changeset: 4832ec2c18a93347ba9f45c171fef7f20c6fc29f

comment:8 Changed 7 years ago by Samuel Jackson

Refs #8116 Move calculation of tsec.

Changeset: 8ea6f971c72c3c0d54ba05f04f6bac5e698d9d59

comment:9 Changed 7 years ago by Samuel Jackson

Refs #8116 Fix for system test.

Changeset: 29258bc6e10531e9668f98fc5d15ffd645e25dd2

comment:10 Changed 7 years ago by Samuel Jackson

Refs #8116 Correction for system test.

Changeset: 48ed2cc8060ac8023b63b11c718ae7e6f71dfa80

comment:11 Changed 7 years ago by Samuel Jackson

Merge branch 'feature/8116_fltabs_code_tidy' of

github.com:mantidproject/mantid into feature/8116_fltabs_code_tidy

Conflicts:

Code/Mantid/scripts/Inelastic/IndirectAbsCor.py

Refs #8116

Changeset: 0236140938b6ba72587a30cc4fd70d13e625ed19

comment:12 Changed 7 years ago by Samuel Jackson

Merge branch 'feature/8116_fltabs_code_tidy' into develop

Conflicts:

Code/Mantid/scripts/Inelastic/IndirectAbsCor.py

Refs #8116

Changeset: 04429e6e2e815b0f22d28e6405143e1b642297fb

comment:13 Changed 7 years ago by Samuel Jackson

Status changed from inprogress to verify
Resolution set to fixed
Tester set to Gesner Passos

I'd like Gesner as the tester for this ticket because we've already been discussing this ticket and I know he likes to be picky with my python code :)

To Tester: As above. There's also an additional system test so make sure that's passing too. This will need to be built using Windows in Release mode for testing. Remember to merge both the system tests and main branch.

comment:14 Changed 7 years ago by Samuel Jackson

Milestone changed from Backlog to Release 3.1

comment:15 Changed 7 years ago by Samuel Jackson

Status changed from verify to reopened
Resolution fixed deleted

comment:16 Changed 7 years ago by Samuel Jackson

Status changed from reopened to inprogress

Refs #8116 Removed skipping of system test for fltabs.

Changeset: 8d5a388204353ac93410b4c19cf6eced143d5d22

comment:17 Changed 7 years ago by Samuel Jackson

Refs #8116 Add check for platform.

Changeset: 66f61226b203a7e2645b399a8a4c3e2eb7d77a3d

comment:18 Changed 7 years ago by Samuel Jackson

Status changed from inprogress to verify
Resolution set to fixed

comment:19 Changed 7 years ago by Owen Arnold

Status changed from verify to verifying
Tester changed from Gesner Passos to Owen Arnold

comment:20 Changed 7 years ago by Owen Arnold

Status changed from verifying to reopened
Resolution fixed deleted

Merge conflicts need fixing.

comment:21 Changed 7 years ago by Samuel Jackson

Status changed from reopened to inprogress

Merge branch 'master' into feature/8116_fltabs_code_tidy

Conflicts:

Code/Mantid/scripts/Inelastic/IndirectAbsCor.py

Refs #8116

Changeset: 76cecb97343ac87e61666857c36a3603ddd2218d

comment:22 Changed 7 years ago by Samuel Jackson

Status changed from inprogress to verify
Resolution set to fixed

comment:23 Changed 7 years ago by Gesner Passos

I did not quite understand why it is still requiring to be tested only on windows machine! I will talk to Samuel on Monday.

comment:24 Changed 7 years ago by Samuel Jackson

It's because IndirectAbsCor still uses a f2py module called cylabs. These modules are only supported in the windows version of Mantid. So while this specific part of the code doesn't require it to be run with Windows, it won't work as IndirectAbsCor checks whether f2py is imported before doing anything else.

Perhaps it would be better to move the import and checking statements to within the body of the code? This would mean flat geometry is supported on all platforms, but leaving cyclic geometry only being supported by windows.