Multipexing-hkem: allows the use of multiple "anatomical" images #424

danieldeidda · 2019-11-21T10:32:35Z

This needs to be merged before the "only_functional" pull request! @ashgillman

Daniel: add list-mode reconstruction by frame file definition

… and sigma_m

ashgillman

Very nice! Looks good to me, just one small indentation recommendation.

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

ashgillman · 2019-11-22T15:25:08Z

Actually, there are merge conflicts.

<<<<<<< MHKEM
  std::vector<shared_ptr<TargetT> > &get_anatomical_prior_sptr();
=======
  shared_ptr<const TargetT> get_anatomical_prior_sptr() const;
>>>>>>> master

Recommend:

  std::vector<shared_ptr<TargetT> > get_anatomical_prior_sptr() const;

<<<<<<< MHKEM
  void set_anatomical_prior_sptr(shared_ptr<TargetT>&, int &index);
  void set_anatomical_image_filenames(std::string&, int &index);
=======
  void set_anatomical_prior_sptr(shared_ptr<TargetT>);

  void set_anatomical_image_filenames(const std::string&);
>>>>>>> master

Recommend:

  void set_anatomical_prior_sptr(shared_ptr<TargetT>, int index);
  void set_anatomical_image_filenames(const std::string&, int index);

<<<<<<< MHKEM
std::vector<shared_ptr<TargetT> > &KOSMAPOSLReconstruction<TargetT>::get_anatomical_prior_sptr()
=======
shared_ptr<const TargetT> KOSMAPOSLReconstruction<TargetT>::get_anatomical_prior_sptr() const
>>>>>>> master

Not sure here... The & should go from the top one, but should it also have a const?

<<<<<<< MHKEM
set_anatomical_prior_sptr (shared_ptr<TargetT>& arg, int &index)
=======
set_anatomical_prior_sptr (shared_ptr<TargetT> arg)
>>>>>>> master

Recommend:

set_anatomical_prior_sptr (shared_ptr<TargetT>arg, int &index)

ashgillman · 2019-11-22T15:26:42Z

Not sure why the Appveyor failed, did it build locally?

ashgillman · 2019-11-22T15:28:44Z

Also, this appears to solve #416, as the code looks like it would gracefully accept an empty list?

danieldeidda · 2019-11-22T18:43:34Z

Not sure why the Appveyor failed, did it build locally?

not sure but it runs on my computer

ashgillman · 2019-12-02T09:59:01Z

The build error is:

/home/travis/build/UCL/STIR/src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx:218:7: error: no matching function for call to ‘std::vector<boost::shared_ptr<stir::DiscretisedDensity<3, float> >, std::allocator<boost::shared_ptr<stir::DiscretisedDensity<3, float> > > >::push_back(std::auto_ptr<stir::DiscretisedDensity<3, float> >)’

       this->anatomical_prior_sptr.push_back(read_from_file<TargetT>(anatomical_image_filenames[i]));

       ^

But strangely only occurs when building on Linux with -DDISABLE_CERN_ROOT=1.

Is this an issue with shared_ptr vs auto_ptr?

ashgillman · 2019-12-02T11:19:46Z

@KrisThielemans
@danieldeidda and I are scratching our heads a little here. Daniel is unable to reproduce locally.

Pointers from std not boost are now preferred, correct? Any idea why DISABLE_CERN_ROOT would change the behaviour?

robbietuk · 2019-12-02T11:38:48Z

if not DISABLE_CERN_ROOT enables cxx11:

STIR/CMakeLists.txt

Lines 103 to 115 in 71c03a1

    
           if(NOT DISABLE_CERN_ROOT) 
        
             find_package(CERN_ROOT) 
        
             if (CERN_ROOT_FOUND) 
        
               message(STATUS "ROOT Version: ${CERN_ROOT_VERSION}") 
        
               # Find which major version this is 
        
               string(REPLACE "." ";" _VERSION_LIST ${CERN_ROOT_VERSION}) 
        
               list(GET _VERSION_LIST 0 CERN_ROOT_VERSION_MAJOR) 
        
               if (${CERN_ROOT_VERSION_MAJOR} GREATER 5) 
        
                 message(STATUS "This ROOT version needs C++-11, so we will enable this.") 
        
                 UseCXX(11) 
        
               endif() 
        
             endif() 
        
           endif()

Perhaps it is a cxx version problem?

ashgillman · 2019-12-02T12:01:14Z

Very likely so, I'd say. But this should be the same for Daniel building locally. I'd say its also a compiler version thing then? But it doesn't explain why a boost pointer is being used..

…

On Mon, 2 Dec 2019 at 12:38, robbietuk ***@***.***> wrote: if not DISABLE_CERN_ROOT enables cxx11: https://github.com/UCL/STIR/blob/71c03a15b1823ad7af010f5a06afcab3131c9664/CMakeLists.txt#L103-L115 Perhaps it is a cxx version problem? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#424?email_source=notifications&email_token=AAGHNCKAUJC22ETQER6DVS3QWTXURA5CNFSM4JQAGLB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFTGR3A#issuecomment-560359660>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGHNCPOBIVTB3DRE6XU5IDQWTXURANCNFSM4JQAGLBQ> .

rijobro · 2019-12-02T13:28:56Z

But this should be the same for Daniel building locally.

What do you mean by this?

STIR should be compatible with pre-C++11. So I think that the DISABLE_CERN_ROOT isn't causing the problem. It's just flagging it up, since C++11 gets disabled when DISABLE_CERN_ROOT=OFF.

KrisThielemans · 2019-12-02T13:42:02Z

As @rijobro almost says, DISABLE_CERN_ROOT=ON means no particular C++ standard is asked for, so it's up to the compiler default to choose one. On Travis, there's some ancient compilers (gcc4 etc) that default to pre-C++11. Hardly anybody has these anymore...

In any case, it's a pre-C++11 bug. you cannot have a vector<auto_ptr<T> >, see e.g. https://stackoverflow.com/questions/111478/why-is-it-wrong-to-use-stdauto-ptr-with-standard-containers.

Probably best is to work-around it like

 shared_ptr<TargetT> an_im_sptr(read_from_file<TargetT>(anatomical_image_filenames[i]));
 this->anatomical_prior_sptr.push_back( an_im_sptr);

and make anatomical_prior_sptr a vector<shared_ptr<...>>.

KrisThielemans

can we make this somehow backwards compatible for SIRF. SIRF 2.1 was released with STIR 4.0.0-alpha. If we change interface between 4.0.0-alpha and 4.0.0, it'll be somewhat confusing...

might be doable with overloading and default arguments of functions.

KrisThielemans · 2019-12-02T13:59:44Z

examples/samples/KOSMAPOSL_mhkem.par

+do symmetry 90degrees min phi := 1
+do_symmetry_180degrees_min_phi:= 1
+do_symmetry_swap_s:= 1
+do_symmetry_swap_segment:= 1
+do_symmetry_shift_z:= 1


I'd rather not have these here. confusing for most people

KrisThielemans · 2019-12-02T14:00:05Z

examples/samples/KOSMAPOSL_mhkem.par

+
+input file := my_noisy_prompts_1ax_pos.hs
+
+; Daniel: here we have the possibility to choose the parameters which define the kernel matrix and the name of the MR image.


remove "Daniel" (I mean the string, not the person!)

KrisThielemans · 2019-12-02T14:01:53Z

src/include/stir/KOSMAPOSL/KOSMAPOSLReconstruction.h

  void set_num_neighbours(const int);
  void set_num_non_zero_feat(const int);
-  void set_sigma_m(const double);
+  void set_sigma_m(double&, int &index);


please no references in arguments of a set function

KrisThielemans · 2019-12-02T14:03:37Z

src/include/stir/KOSMAPOSL/KOSMAPOSLReconstruction.h

@@ -235,12 +235,12 @@ class KOSMAPOSLReconstruction:

  /*! Create a matrix similarly to calculate_norm_matrix() but this is done for the anatomical image, */
  /*! which does not  change over iteration.*/
-    void calculate_norm_const_matrix(TargetT &normm,
+    void calculate_norm_const_matrix(std::vector<shared_ptr<TargetT> > normm,


this doesn't have any output apparently, but only side-effects? is that correct? best to document. Please make normm a reference, const if input`, non-const if output.

yes normm is the output, i forgot the reference

KrisThielemans · 2019-12-02T14:04:37Z

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

@@ -170,17 +171,14 @@ initialise_keymap()
 //  this->parser.add_key("anatomical image filename",&this->anatomical_image_filenames);
  this->parser.add_key("number of neighbours",&this->num_neighbours);
  this->parser.add_key("number of non-zero feature elements",&this->num_non_zero_feat);
-  this->parser.add_key("sigma_m",&this->sigma_m);
+  this->parser.add_key("sigma_m",&sigma_m);


why not this->?

no reason, I probably forgot it

KrisThielemans · 2019-12-02T14:07:55Z

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

 }

 template <typename TargetT>
 void
 KOSMAPOSLReconstruction<TargetT>::
-set_anatomical_image_filenames(const std::string& arg)
+set_anatomical_image_filenames(std::string &arg, int index)


arg is an input argument, need to have it const

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

examples/samples/KOSMAPOSL_mhkem.par

KrisThielemans · 2019-12-02T14:16:28Z

src/include/stir/KOSMAPOSL/KOSMAPOSLReconstruction.h

@@ -196,7 +196,7 @@ class KOSMAPOSLReconstruction:
  //! Anatomical image filename
  std::vector<std::string> anatomical_image_filenames;

-  std::vector<shared_ptr<TargetT> > anatomical_prior_sptr,kmnorm_sptr;
+  std::vector<std::shared_ptr<TargetT> > anatomical_prior_sptr,kmnorm_sptr;


no std::shared_ptr please. it'll break. just shared_ptr

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

KrisThielemans · 2019-12-19T07:16:27Z

@danieldeidda is this for next year? I might try to get 4.0 beta out during the holidays...

danieldeidda

@danieldeidda is this for next year? I might try to get 4.0 beta out during the holidays...

Sorry I left it bit. This can be done in these days, I will review the comments and apply the modification. I remember we said something about the sigma_m being a vector now that can be an issue for SIRF

danieldeidda · 2019-12-02T16:42:34Z

src/include/stir/KOSMAPOSL/KOSMAPOSLReconstruction.h

@@ -235,12 +235,12 @@ class KOSMAPOSLReconstruction:

  /*! Create a matrix similarly to calculate_norm_matrix() but this is done for the anatomical image, */
  /*! which does not  change over iteration.*/
-    void calculate_norm_const_matrix(TargetT &normm,
+    void calculate_norm_const_matrix(std::vector<shared_ptr<TargetT> > normm,


yes normm is the output, i forgot the reference

danieldeidda · 2019-12-02T16:44:16Z

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

@@ -170,17 +171,14 @@ initialise_keymap()
 //  this->parser.add_key("anatomical image filename",&this->anatomical_image_filenames);
  this->parser.add_key("number of neighbours",&this->num_neighbours);
  this->parser.add_key("number of non-zero feature elements",&this->num_non_zero_feat);
-  this->parser.add_key("sigma_m",&this->sigma_m);
+  this->parser.add_key("sigma_m",&sigma_m);


no reason, I probably forgot it

danieldeidda · 2019-12-02T16:45:52Z

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

-  if (this->anatomical_image_filenames.size()>1){
-      error("At the moment you can only use one anatomical image %s");
+  if (!this->anatomical_image_filenames.size()==sigma_m.size()){
+      error("The number of sigma_m parameters must be the same as the numberof anatomical image filenames %s");


probably best not to put any argument

KrisThielemans

travis etc are all happy so this is great. I have a few tiny ones left. Please check if SIRF 2.1 compiles/runs with this version as wel (it probably does)l. Then good to go.

src/include/stir/KOSMAPOSL/KOSMAPOSLReconstruction.h

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx

…the same value

danieldeidda · 2020-01-13T14:12:43Z

SIRF compiles with the last commit

KrisThielemans · 2020-01-14T10:52:25Z

thanks @danieldeidda

by the way, ci skip needs to be [ci skip] for it to prevent CI running...

danieldeidda · 2020-01-14T17:04:08Z

Ops! :)

…

________________________________ From: Kris Thielemans <notifications@github.com> Sent: Tuesday, January 14, 2020 10:52:26 AM To: UCL/STIR <STIR@noreply.github.com> Cc: Daniel Deidda <daniel.deidda@npl.co.uk>; Mention <mention@noreply.github.com> Subject: [EXTERNAL] Re: [UCL/STIR] Multipexing-hkem: allows the use of multiple "anatomical" images (#424) CAUTION: This email originated from outside of NPL. Do not click links or open attachments unless you recognize the sender and know the content is safe. thanks @danieldeidda<https://github.com/danieldeidda> by the way, ci skip needs to be [ci skip] for it to prevent CI running... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#424?email_source=notifications&email_token=AECUSJPSOSO4TXRFIQTEUDLQ5WKOVA5CNFSM4JQAGLB2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI4FLFQ#issuecomment-574117270>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AECUSJKKX46SSKRPKTHB3G3Q5WKOVANCNFSM4JQAGLBQ>. Visit the NPL website<https://www.npl.co.uk/> and find out how our cutting-edge measurement science has a positive impact in the real world.

________________________________ [http://resource.npl.co.uk/images/npl-logo.jpg]<http://www.npl.co.uk/?utm_source=Email%20Footer&utm_medium=email&utm_campaign=Homepage> Keep in touch<http://www.npl.co.uk/contact> [http://resource.npl.co.uk/images/twitter.jpg] <http://www.twitter.com/npl> [http://resource.npl.co.uk/images/facebook.jpg] <http://www.facebook.com/npldigital> [http://resource.npl.co.uk/images/youtube.jpg] <http://www.youtube.com/npldigital>

________________________________ NPL Privacy Policy<http://www.npl.co.uk/privacy-policy/> If you have received this message in error, please notify us and remove it from your system. NPL Management Ltd cannot guarantee that the e-mail or any attachments are free from viruses. NPL Management Ltd is a company registered in England and Wales, number: 2937881 Registered office: National Physical Laboratory | Hampton Road | Teddington, Middlesex | UK | TW11 0LW

danieldeidda and others added 10 commits May 12, 2018 20:19

Add files via upload

dde4920

Daniel: add list-mode reconstruction by frame file definition

Merge branch 'master' of git://github.com/UCL/STIR

bc63560

Merge branch 'master' of git://github.com/UCL/STIR

c1b6b4f

Merge branch 'master' of git://github.com/UCL/STIR

95e71b1

Merge remote-tracking branch 'upstream/master'

8d8a75b

merge master

fb04d94

Made sigma_m, anatomical_sd vectors and run a loop for all the images…

108723d

… and sigma_m

build working but recon is not, need debug

a85e669

small modifications and added parfile, still not running

2e39ac7

vector issues fixed, this is running correctly, but a bit slow

a467dd4

ashgillman approved these changes Nov 22, 2019

View reviewed changes

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx Outdated Show resolved Hide resolved

ashgillman added the enhancement label Nov 22, 2019

Merge branch 'master' into MHKEM

0c34d4a

make read_from_file output a shared_ptr

c6c78c0

KrisThielemans requested changes Dec 2, 2019

View reviewed changes

danieldeidda commented Dec 19, 2019

View reviewed changes

danieldeidda added 2 commits December 19, 2019 10:12

Merge branch 'master' into MHKEM

bf71288

add keep set sigmam and set anatomical filename as compatible with SIRF

914d5a3

KrisThielemans requested changes Jan 6, 2020

View reviewed changes

src/include/stir/KOSMAPOSL/KOSMAPOSLReconstruction.h Outdated Show resolved Hide resolved

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx Show resolved Hide resolved

src/iterative/KOSMAPOSL/KOSMAPOSLReconstruction.cxx Outdated Show resolved Hide resolved

danieldeidda added 3 commits January 7, 2020 15:57

small changes

e868649

added set_anatomical_prior_sptr(arg) setting all vector elements to …

f4ed18f

…the same value

remove & from set_anatomical_prior_sptr

5cb45b4

ci skip add reference and doxygen comments

bd775cd

KrisThielemans merged commit 83a35bb into UCL:master Jan 14, 2020

ashgillman mentioned this pull request Jan 30, 2020

Only functional hkem #422

Closed

danieldeidda deleted the MHKEM branch April 15, 2020 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipexing-hkem: allows the use of multiple "anatomical" images #424

Multipexing-hkem: allows the use of multiple "anatomical" images #424

danieldeidda commented Nov 21, 2019

ashgillman left a comment

ashgillman commented Nov 22, 2019

ashgillman commented Nov 22, 2019

ashgillman commented Nov 22, 2019

danieldeidda commented Nov 22, 2019

ashgillman commented Dec 2, 2019

ashgillman commented Dec 2, 2019

robbietuk commented Dec 2, 2019

ashgillman commented Dec 2, 2019 via email •

edited

rijobro commented Dec 2, 2019

KrisThielemans commented Dec 2, 2019

KrisThielemans left a comment

KrisThielemans Dec 2, 2019

KrisThielemans Dec 2, 2019

KrisThielemans Dec 2, 2019

KrisThielemans Dec 2, 2019

danieldeidda Dec 2, 2019

KrisThielemans Dec 2, 2019

danieldeidda Dec 2, 2019

KrisThielemans Dec 2, 2019

KrisThielemans Dec 2, 2019

KrisThielemans commented Dec 19, 2019

danieldeidda left a comment •

edited

danieldeidda Dec 2, 2019

danieldeidda Dec 2, 2019

danieldeidda Dec 2, 2019

KrisThielemans left a comment

danieldeidda commented Jan 13, 2020

KrisThielemans commented Jan 14, 2020

danieldeidda commented Jan 14, 2020 via email


		input file := my_noisy_prompts_1ax_pos.hs

		; Daniel: here we have the possibility to choose the parameters which define the kernel matrix and the name of the MR image.

Multipexing-hkem: allows the use of multiple "anatomical" images #424

Multipexing-hkem: allows the use of multiple "anatomical" images #424

Conversation

danieldeidda commented Nov 21, 2019

ashgillman left a comment

Choose a reason for hiding this comment

ashgillman commented Nov 22, 2019

ashgillman commented Nov 22, 2019

ashgillman commented Nov 22, 2019

danieldeidda commented Nov 22, 2019

ashgillman commented Dec 2, 2019

ashgillman commented Dec 2, 2019

robbietuk commented Dec 2, 2019

ashgillman commented Dec 2, 2019 via email • edited

rijobro commented Dec 2, 2019

KrisThielemans commented Dec 2, 2019

KrisThielemans left a 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

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

KrisThielemans commented Dec 19, 2019

danieldeidda left a comment • edited

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

KrisThielemans left a comment

Choose a reason for hiding this comment

danieldeidda commented Jan 13, 2020

KrisThielemans commented Jan 14, 2020

danieldeidda commented Jan 14, 2020 via email

ashgillman commented Dec 2, 2019 via email •

edited

danieldeidda left a comment •

edited