Sounds like an awesome undertaking. I know niftyreg is huge (hundreds of thousands of lines) so 14k isn't bad by comparison.

I haven't actually looked at the code in detail. About fullfile in matlab, there's also filesep and pathsep which you may find useful.

Casper

Hi David, Casper,

 

Thanks both for taking the time to look at the code. I’ll copy these answers below onto the PR. Would it be possible to make any future comments over there (link) so that they don’t get lost? Thanks.

 

David,

• Good spot for the spelling in the first point
• The get/set functions were written by Evgueni and they exist in mSTIR and mGadgetron, so I copied them over to mSIRFReg, too. I don’t know what the difference is between mutilities, miutilities, etc., but everything seems to work.
• OK for fullfile in Matlab
• OK for output_path -> output_prefix
• The American spelling is only used when wrapping NiftyReg. In the case of SetInterpolationToNearestNeighbor , it’s a callback function, so I have to match their spelling (it would only be used in a parameter file). When I’m adding my own functionality, however, I spell it the correct way ;)
• Ok for the last point on missing documentation.

 

Casper, 

I’m not sure which part you’re referring to as copy-paste. The wrapping into Matlab and Python? Or the wrapping of NiftyReg itself at the C++ level?

• If it was the first, it’s not copy-paste. To get the same functionality, you have to write the C-interface and then the relevant python and Matlab classes (as discussed here). So obviously the goal is to get duplicated functionality in the different languages, but it’s certainly (and unfortunately) not a copy-paste job! 
• If it was the second, I haven’t copied any source code from NiftyReg. These are wrappers around the NiftyReg classes (with a fair bit of new functionality), so no copy-paste there either. 

 

Kind regards,

Richard

On 28 Sep 2018, at 11:19, Casper da Costa-Luis <[log in to unmask]> wrote:

 

That's why I'd assumed it was a copy-paste job rather than a proper wrapping. 14k indeed. If there's some pattern to the wrapping maybe we could make use of cmake's

configure_file?

 

On Fri, 28 Sep 2018 at 08:46, Atkinson, David <[log in to unmask]> wrote:

14,000+ lines of code?!! That must have been a lot of work!  

 

 

I spotted:

src/Registration/mReg/libload_sirfreg.m

error message mentions mutil… but it seems to be miutil…

 

src/Registration/mReg/+mSIRFReg/setParameter.m

has mutilities, mUtilities and miutilities – in either code or comments – are these all correct?

 

src/Registration/mReg/tests/test_mSIRFReg.m

You should use fullfile to compose filenames with paths – this provides robustness against the / and \ file separators on different systems and I think fullfile acts sensibly when there is one missing, or one too many file separators.

 

Variable output_path   might be confusingly named as it appears to be both a path and part of the filename.

 

In src/Registration/cReg/SIRFRegNiftyAladinSym.cpp

You have the American spelling of ‘neighbor’,  but elsewhere you use the English ‘neighbour’

parser.add_key      ( "SetInterpolationToNearestNeighbor",  &reg_aladin<T>::SetInterpolationToNearestNeighbor

 

src/Registration/cReg/SIRFReg.h

comment about need to give path on line 41

 

 

 

 

 

 

From: CCP <[log in to unmask]> on behalf of "Brown, Richard" <[log in to unmask]>
Reply-To: "Brown, Richard" <[log in to unmask]>
Date: Thursday, 27 September 2018 at 16:19
To: CCP <[log in to unmask]>
Subject: SIRFReg code review
Resent-From: David Atkinson <[log in to unmask]>

 

Dear all,

 

We’ll be having a code review next Friday (5/10) 9-11am of SIRFReg (the wrapping of NityReg into SIRF to enable registration and resampling). 

 

Please note that we’ll be discussing the class interface, and not necessarily the actual implementation. 

 

The pull request can be found here. Should you wish to get involved, it would be great if you could give the code a look-over ahead of the meeting (and feel free to comment/create issues).

 

Thanks in advance.

 

Kind regards,

Richard Brown

 

---

To unsubscribe from the CCP-PETMR-DEVEL list, click the following link:
https://www.jiscmail.ac.uk/cgi-bin/webadmin?SUBED1=CCP-PETMR-DEVEL&A=1

 

---

To unsubscribe from the CCP-PETMR-DEVEL list, click the following link:
https://www.jiscmail.ac.uk/cgi-bin/webadmin?SUBED1=CCP-PETMR-DEVEL&A=1

 

---

To unsubscribe from the CCP-PETMR-DEVEL list, click the following link:
https://www.jiscmail.ac.uk/cgi-bin/webadmin?SUBED1=CCP-PETMR-DEVEL&A=1