Dicom package / isdicom function

classic Classic list List threaded Threaded
22 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Dicom package / isdicom function

adama

Hi,

I've written isdicom (as an .m file), a function which is currently missing from the Dicom package.  If it's likely to be useful then I'm happy to submit it to Octave-forge (is there a different mailing list for Octave-forge?), or I could forward the code to the maintainer of the package if that's more helpful.

Adam
 

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

andy buckle
On 13 September 2012 12:24, adam aitkenhead <[hidden email]> wrote:

>
> Hi,
>
> I've written isdicom (as an .m file), a function which is currently missing
> from the Dicom package.  If it's likely to be useful then I'm happy to
> submit it to Octave-forge (is there a different mailing list for
> Octave-forge?), or I could forward the code to the maintainer of the package
> if that's more helpful.
>
> Adam

If you email to me, I will take a look, and commit it to the svn repo.
Are you OK with GPL V3 or higher? Do you want your name in the
comments at the top of the file?

Thanks,

Andy (dicom package maintainer)

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

andy buckle
>> Date: Thu, 13 Sep 2012 14:57:29 +0100
>
>> Subject: Re: [OctDev] Dicom package / isdicom function
>> From: [hidden email]
>> To: [hidden email]
>>
>> > I have access to Matlab too, and it recognises the non-standard DICOM
>> > file.
>> > (Just to note, although I have tested the ML behaviour for a some test
>> > files, I didn't use Matlab as a basis for the code.)
>>
>> Excellent. That is the way to do it. We are careful about copyright.
>>
>> Andy
On 19 September 2012 17:26, adam aitkenhead <[hidden email]> wrote:

>
> Hi Andy,
>
> I've attached an updated version of the isdicom function which can now check
> a list of files (in a cell array) in one go, which is much quicker than
> checking each file separately.  Again, no rush for releasing a new version
> of the toolbox - just some changes I was making for my own code anyway.
>
> Also on a different note, I've written functions which read/write the
> Analyze format, giving functions equivalent to Matlab's analyze75info and
> analyze75read.  Would you rather keep the Dicom toolbox purely for the Dicom
> format, or are you interested in expanding it to become a general Medical
> File Format toolbox?  No worries if not, just thought I'd see what you
> thought before I see where else they could fit into Octave-forge.
>
> Adam

Note that I have included the list for further discussion, and edited
older text to chronological order.

I have not used analyze75. If it is just m-files, then the image
package is probably a good place. The main reason dicom is on its own
is that it has burdensome dependencies.

--
/* andy buckle */

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://ad.doubleclick.net/clk;258768047;13503038;j?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

krthie

> From: Andy Buckle
> Sent: 19 September 2012 19:01
> >
> > From adamaitkenhead

> > Also on a different note, I've written functions which read/write the
> > Analyze format, giving functions equivalent to Matlab's analyze75info
and
> > analyze75read.  Would you rather keep the Dicom toolbox purely for the
> Dicom
> > format, or are you interested in expanding it to become a general
Medical
> > File Format toolbox?  No worries if not, just thought I'd see what you
> > thought before I see where else they could fit into Octave-forge.
> >
> > Adam
>
> I have not used analyze75. If it is just m-files, then the image
> package is probably a good place. The main reason dicom is on its own
> is that it has burdensome dependencies.
>

I'd keep it out of the dicom package as well.

Note that there's a number of packages out there to read nifti files (which
is a superset of analyze75, see http://nifti.nimh.nih.gov/nifti-1/), not all
with very clear license though. One that seems ok is
http://www.nitrc.org/projects/cbinifti/, but I haven't used it (it might
rely on unix pipes) . Then there's http://niftilib.sourceforge.net/ which
also provides matlab support, but as it's the IO basis for SPM, and a lot of
SPM works on Octave, it might work.

Of course, there's still a place for analyze75info etc compatible functions.
I have no idea if that already exists somewhere else.

Kris


------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://ad.doubleclick.net/clk;258768047;13503038;j?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

Carnë Draug-2
In reply to this post by andy buckle
On 19 September 2012 20:00, Andy Buckle <[hidden email]> wrote:

>>> Date: Thu, 13 Sep 2012 14:57:29 +0100
>>
>>> Subject: Re: [OctDev] Dicom package / isdicom function
>>> From: [hidden email]
>>> To: [hidden email]
>>>
>>> > I have access to Matlab too, and it recognises the non-standard DICOM
>>> > file.
>>> > (Just to note, although I have tested the ML behaviour for a some test
>>> > files, I didn't use Matlab as a basis for the code.)
>>>
>>> Excellent. That is the way to do it. We are careful about copyright.
>>>
>>> Andy
> On 19 September 2012 17:26, adam aitkenhead <[hidden email]> wrote:
>>
>> Hi Andy,
>>
>> I've attached an updated version of the isdicom function which can now check
>> a list of files (in a cell array) in one go, which is much quicker than
>> checking each file separately.  Again, no rush for releasing a new version
>> of the toolbox - just some changes I was making for my own code anyway.
>>
>> Also on a different note, I've written functions which read/write the
>> Analyze format, giving functions equivalent to Matlab's analyze75info and
>> analyze75read.  Would you rather keep the Dicom toolbox purely for the Dicom
>> format, or are you interested in expanding it to become a general Medical
>> File Format toolbox?  No worries if not, just thought I'd see what you
>> thought before I see where else they could fit into Octave-forge.
>>
>> Adam
>
> Note that I have included the list for further discussion, and edited
> older text to chronological order.
>
> I have not used analyze75. If it is just m-files, then the image
> package is probably a good place. The main reason dicom is on its own
> is that it has burdensome dependencies.

I agree with Andy. The analyze75read/write functions would fit better
on the image package. Are their API matlab compatible?

Carnë

------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://ad.doubleclick.net/clk;258768047;13503038;j?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

adama
Hi all,

I see what you mean about the dicom package's dependencies, sounds like the image package is a better place for it.

The underlying read function has a different calling syntax from Matlab, as it reads both the header and the image in one go (I wrote it like that before I realized there were similar functions in Matlab).  However I've also written functions which call it using the same format as analyze75info and analyze75read.  This does mean that it runs slower than Matlab, but it shouldn't be difficult to change it to read only the appropriate parts if needed.

I've only been able to test it with my own Analyze files, so it's not been tested using other files with different orientations or anything like that.

Adam

 


> From: [hidden email]

> Date: Thu, 20 Sep 2012 12:33:35 +0200
> Subject: Re: [OctDev] Dicom package / isdicom function
> To: [hidden email]
> CC: [hidden email]; [hidden email]
>
> On 19 September 2012 20:00, Andy Buckle <[hidden email]> wrote:
> >>> Date: Thu, 13 Sep 2012 14:57:29 +0100
> >>
> >>> Subject: Re: [OctDev] Dicom package / isdicom function
> >>> From: [hidden email]
> >>> To: [hidden email]
> >>>
> >>> > I have access to Matlab too, and it recognises the non-standard DICOM
> >>> > file.
> >>> > (Just to note, although I have tested the ML behaviour for a some test
> >>> > files, I didn't use Matlab as a basis for the code.)
> >>>
> >>> Excellent. That is the way to do it. We are careful about copyright.
> >>>
> >>> Andy
> > On 19 September 2012 17:26, adam aitkenhead <[hidden email]> wrote:
> >>
> >> Hi Andy,
> >>
> >> I've attached an updated version of the isdicom function which can now check
> >> a list of files (in a cell array) in one go, which is much quicker than
> >> checking each file separately. Again, no rush for releasing a new version
> >> of the toolbox - just some changes I was making for my own code anyway.
> >>
> >> Also on a different note, I've written functions which read/write the
> >> Analyze format, giving functions equivalent to Matlab's analyze75info and
> >> analyze75read. Would you rather keep the Dicom toolbox purely for the Dicom
> >> format, or are you interested in expanding it to become a general Medical
> >> File Format toolbox? No worries if not, just thought I'd see what you
> >> thought before I see where else they could fit into Octave-forge.
> >>
> >> Adam
> >
> > Note that I have included the list for further discussion, and edited
> > older text to chronological order.
> >
> > I have not used analyze75. If it is just m-files, then the image
> > package is probably a good place. The main reason dicom is on its own
> > is that it has burdensome dependencies.
>
> I agree with Andy. The analyze75read/write functions would fit better
> on the image package. Are their API matlab compatible?
>
> Carnë

------------------------------------------------------------------------------
Got visibility?
Most devs has no idea what their production app looks like.
Find out how fast your code is with AppDynamics Lite.
http://ad.doubleclick.net/clk;262219671;13503038;y?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

Carnë Draug-2
On 21 September 2012 10:18, adam aitkenhead <[hidden email]> wrote:
> The underlying read function has a different calling syntax from Matlab, as
> it reads both the header and the image in one go (I wrote it like that
> before I realized there were similar functions in Matlab).  However I've
> also written functions which call it using the same format as analyze75info
> and analyze75read.  This does mean that it runs slower than Matlab, but it
> shouldn't be difficult to change it to read only the appropriate parts if
> needed.

It's ok to do more than the matlab functions, there's plenty of
examples where that is the case. It's just not very good to have
conflicts with their API. So if your analyze75read works as in
"[image, header] = analyze75read (fname)", it's still matlab
compatible while being more useful at the same time. It seems to me
that matlab also has a analyze75info function, maybe you'd like to
make it compatible? I'm planning on making a release of the image
package Sunday in case you are interested in having your functions
included now. Otherwise they can still be included on the next
release.

Carnë

------------------------------------------------------------------------------
Got visibility?
Most devs has no idea what their production app looks like.
Find out how fast your code is with AppDynamics Lite.
http://ad.doubleclick.net/clk;262219671;13503038;y?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

adama
I probably won't have a chance to send it before Sunday (the files are on my laptop which I don't have with me this weekend).  But I should be able to send it next week, so the following release should be no problem.
Thanks,
Adam

 


> From: [hidden email]

> Date: Fri, 21 Sep 2012 13:53:56 +0200
> Subject: Re: [OctDev] Dicom package / isdicom function
> To: [hidden email]
> CC: [hidden email]; [hidden email]
>
> On 21 September 2012 10:18, adam aitkenhead <[hidden email]> wrote:
> > The underlying read function has a different calling syntax from Matlab, as
> > it reads both the header and the image in one go (I wrote it like that
> > before I realized there were similar functions in Matlab). However I've
> > also written functions which call it using the same format as analyze75info
> > and analyze75read. This does mean that it runs slower than Matlab, but it
> > shouldn't be difficult to change it to read only the appropriate parts if
> > needed.
>
> It's ok to do more than the matlab functions, there's plenty of
> examples where that is the case. It's just not very good to have
> conflicts with their API. So if your analyze75read works as in
> "[image, header] = analyze75read (fname)", it's still matlab
> compatible while being more useful at the same time. It seems to me
> that matlab also has a analyze75info function, maybe you'd like to
> make it compatible? I'm planning on making a release of the image
> package Sunday in case you are interested in having your functions
> included now. Otherwise they can still be included on the next
> release.
>
> Carnë

------------------------------------------------------------------------------
Got visibility?
Most devs has no idea what their production app looks like.
Find out how fast your code is with AppDynamics Lite.
http://ad.doubleclick.net/clk;262219671;13503038;y?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Dicom package / isdicom function

Carnë Draug-2
On 21 September 2012 16:10, adam aitkenhead <[hidden email]> wrote:

>> From: [hidden email]
>> Date: Fri, 21 Sep 2012 13:53:56 +0200
>>
>> On 21 September 2012 10:18, adam aitkenhead <[hidden email]>
>> wrote:
>> > The underlying read function has a different calling syntax from Matlab,
>> > as
>> > it reads both the header and the image in one go (I wrote it like that
>> > before I realized there were similar functions in Matlab). However I've
>> > also written functions which call it using the same format as
>> > analyze75info
>> > and analyze75read. This does mean that it runs slower than Matlab, but
>> > it
>> > shouldn't be difficult to change it to read only the appropriate parts
>> > if
>> > needed.
>>
>> It's ok to do more than the matlab functions, there's plenty of
>> examples where that is the case. It's just not very good to have
>> conflicts with their API. So if your analyze75read works as in
>> "[image, header] = analyze75read (fname)", it's still matlab
>> compatible while being more useful at the same time. It seems to me
>> that matlab also has a analyze75info function, maybe you'd like to
>> make it compatible? I'm planning on making a release of the image
>> package Sunday in case you are interested in having your functions
>> included now. Otherwise they can still be included on the next
>> release.
>>
>> Carnë
>
> I probably won't have a chance to send it before Sunday (the files are on my
> laptop which I don't have with me this weekend).  But I should be able to
> send it next week, so the following release should be no problem.
> Thanks,
> Adam

This is taking me more time than initially expected. If you want,
could still try to add them to this release.

Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

adama
Hi Carnë,

Attached are those m files for analyze75info and analyze75read - I've reformatted them to use Matlab's syntax.  I've still to tidy up analyze75write, but I'll send it on later this week if I can.

Adam


> From: [hidden email]

> Date: Mon, 24 Sep 2012 10:40:17 +0200
> Subject: Re: [OctDev] Dicom package / isdicom function
> To: [hidden email]
> CC: [hidden email]
>
> On 21 September 2012 16:10, adam aitkenhead <[hidden email]> wrote:
> >> From: [hidden email]
> >> Date: Fri, 21 Sep 2012 13:53:56 +0200
> >>
> >> On 21 September 2012 10:18, adam aitkenhead <[hidden email]>
> >> wrote:
> >> > The underlying read function has a different calling syntax from Matlab,
> >> > as
> >> > it reads both the header and the image in one go (I wrote it like that
> >> > before I realized there were similar functions in Matlab). However I've
> >> > also written functions which call it using the same format as
> >> > analyze75info
> >> > and analyze75read. This does mean that it runs slower than Matlab, but
> >> > it
> >> > shouldn't be difficult to change it to read only the appropriate parts
> >> > if
> >> > needed.
> >>
> >> It's ok to do more than the matlab functions, there's plenty of
> >> examples where that is the case. It's just not very good to have
> >> conflicts with their API. So if your analyze75read works as in
> >> "[image, header] = analyze75read (fname)", it's still matlab
> >> compatible while being more useful at the same time. It seems to me
> >> that matlab also has a analyze75info function, maybe you'd like to
> >> make it compatible? I'm planning on making a release of the image
> >> package Sunday in case you are interested in having your functions
> >> included now. Otherwise they can still be included on the next
> >> release.
> >>
> >> Carnë
> >
> > I probably won't have a chance to send it before Sunday (the files are on my
> > laptop which I don't have with me this weekend). But I should be able to
> > send it next week, so the following release should be no problem.
> > Thanks,
> > Adam
>
> This is taking me more time than initially expected. If you want,
> could still try to add them to this release.
>
> Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev

analyze75info.m (9K) Download Attachment
analyze75read.m (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

Carnë Draug-2
On 25 September 2012 11:17, adam aitkenhead <[hidden email]> wrote:
> Hi Carnë,
>
> Attached are those m files for analyze75info and analyze75read - I've
> reformatted them to use Matlab's syntax.  I've still to tidy up
> analyze75write, but I'll send it on later this week if I can.
>
> Adam

Hi Adam

Thank you for the functions. I have just made the commit and added
them to the package. I have added your e-mail to the copyright notice
but let me know if you have another address that you prefer. I can
wait a bit more to have the whole analyze75 set of functions.

If you are already tidying up the next function, would be cool if you
could follow some of the standards of Octave core (most in Octave
Forge don't follow it but I find they good standards). Here's some:

  * indent the function body (this will save from writing end %function)
  * spaces after function names
  * no spaces after variables for indexing
  * user @var for variables name in the help text (do not capitalize them)

I'd suggest the others but it seems to me that you want to keep your
code Matlab compatible. Is that correct?

Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

adama
Hi Carnë

Thanks for the tips, they all sound like a good idea - I'll aim to follow them for future code.  (Yup, I've been trying to keep the code so that it also works in Matlab where possible.)

When you're ready with everything else just go ahead and release the package, I can't guarantee I'll be able to tidy up the remaining function quickly so I don't want to hold things up.

This email address is fine, thanks for adding it.

Thanks,
Adam
 


> From: [hidden email]

> Date: Tue, 25 Sep 2012 14:51:42 +0200
> Subject: Re: [OctDev] Analyze75
> To: [hidden email]
> CC: [hidden email]
>
> On 25 September 2012 11:17, adam aitkenhead <[hidden email]> wrote:
> > Hi Carnë,
> >
> > Attached are those m files for analyze75info and analyze75read - I've
> > reformatted them to use Matlab's syntax. I've still to tidy up
> > analyze75write, but I'll send it on later this week if I can.
> >
> > Adam
>
> Hi Adam
>
> Thank you for the functions. I have just made the commit and added
> them to the package. I have added your e-mail to the copyright notice
> but let me know if you have another address that you prefer. I can
> wait a bit more to have the whole analyze75 set of functions.
>
> If you are already tidying up the next function, would be cool if you
> could follow some of the standards of Octave core (most in Octave
> Forge don't follow it but I find they good standards). Here's some:
>
> * indent the function body (this will save from writing end %function)
> * spaces after function names
> * no spaces after variables for indexing
> * user @var for variables name in the help text (do not capitalize them)
>
> I'd suggest the others but it seems to me that you want to keep your
> code Matlab compatible. Is that correct?
>
> Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

Carnë Draug-2
On 25 September 2012 15:11, adam aitkenhead <[hidden email]> wrote:
> Thanks for the tips, they all sound like a good idea - I'll aim to follow
> them for future code.  (Yup, I've been trying to keep the code so that it
> also works in Matlab where possible.)

I've been changing a bit the input check. The standard we have is to
check the value of nargin. Doing this at the start means that you
don't have to check if the variable exists later.

Another thing I noticed is the many ways it tries to handle invalid
filenames. While it looks kinda slick, I'm not sure it's a good idea
to have the function doing it, it should be done by the script that
calls it. When you give wrong arguments to a function, trying to guess
what is right can lead to weird results. Or does matlab also behaves
that way (it's not documented at least). My suggestion would be to
maybe display the file selection if no argument is given but return an
error if the filenames are not valid files. What do you think?

Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

adama
All sounds good to me.  Makes sense to keep to the standard way of doing things to make it behave as others might expect.  I'm not all that familiar with the standard (yet), so if you see any ways to standardise it that is great.

About the input checks - I'd aimed to make it behave as Matlab does for all acceptable inputs.  But for the cases where Matlab throws an error, this function generally asks the user what to do instead.  But I'm happy for this to be simplified or left for other functions which call these.

 
> From: [hidden email]

> Date: Tue, 25 Sep 2012 16:30:48 +0200
> Subject: Re: [OctDev] Analyze75
> To: [hidden email]
> CC: [hidden email]
>
> I've been changing a bit the input check. The standard we have is to
> check the value of nargin. Doing this at the start means that you
> don't have to check if the variable exists later.
>
> Another thing I noticed is the many ways it tries to handle invalid
> filenames. While it looks kinda slick, I'm not sure it's a good idea
> to have the function doing it, it should be done by the script that
> calls it. When you give wrong arguments to a function, trying to guess
> what is right can lead to weird results. Or does matlab also behaves
> that way (it's not documented at least). My suggestion would be to
> maybe display the file selection if no argument is given but return an
> error if the filenames are not valid files. What do you think?
>
> Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

Carnë Draug-2
hey, please avoid top posting. Reply at the bottom

On 25 September 2012 17:07, adam aitkenhead <[hidden email]> wrote:

>> From: [hidden email]
>> Date: Tue, 25 Sep 2012 16:30:48 +0200
>
>> Another thing I noticed is the many ways it tries to handle invalid
>> filenames. While it looks kinda slick, I'm not sure it's a good idea
>> to have the function doing it, it should be done by the script that
>> calls it. When you give wrong arguments to a function, trying to guess
>> what is right can lead to weird results. Or does matlab also behaves
>> that way (it's not documented at least). My suggestion would be to
>> maybe display the file selection if no argument is given but return an
>> error if the filenames are not valid files. What do you think?
>>
>
> All sounds good to me.  Makes sense to keep to the standard way of doing
> things to make it behave as others might expect.  I'm not all that familiar
> with the standard (yet), so if you see any ways to standardise it that is
> great.

I've made a few more changes. Could you please give them a try to make
sure I didn't broke anything? :p

I've moved the code that was common between read and info into a
shared private function, checks after fseek and fopen, everything in
one function (I see no need to have subfunctions since they are only
called once anyway), replaced a long if block for a switch block,
changed many checks for simpler, used dir to check the file size
(rather than the low level fseek and ftell), and a lot of whitespace
to make it pretty and extended documentation. I tried to make the code
run in matlab (though I'd guess a matlab user would actually just use
the matlab function).

Here's the files:
https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/private/analyze75filename.m
https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/analyze75info.m
https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/analyze75read.m

> About the input checks - I'd aimed to make it behave as Matlab does for all
> acceptable inputs.  But for the cases where Matlab throws an error, this
> function generally asks the user what to do instead.  But I'm happy for this
> to be simplified or left for other functions which call these.

Done.

Carnë

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

adama
> From: [hidden email]
> Date: Tue, 25 Sep 2012 19:34:15 +0200

> Subject: Re: [OctDev] Analyze75
> To: [hidden email]
> CC: [hidden email]
>
> hey, please avoid top posting. Reply at the bottom
>
> On 25 September 2012 17:07, adam aitkenhead <[hidden email]> wrote:
> >> From: [hidden email]
> >> Date: Tue, 25 Sep 2012 16:30:48 +0200
> >
> >> Another thing I noticed is the many ways it tries to handle invalid
> >> filenames. While it looks kinda slick, I'm not sure it's a good idea
> >> to have the function doing it, it should be done by the script that
> >> calls it. When you give wrong arguments to a function, trying to guess
> >> what is right can lead to weird results. Or does matlab also behaves
> >> that way (it's not documented at least). My suggestion would be to
> >> maybe display the file selection if no argument is given but return an
> >> error if the filenames are not valid files. What do you think?
> >>
> >
> > All sounds good to me. Makes sense to keep to the standard way of doing
> > things to make it behave as others might expect. I'm not all that familiar
> > with the standard (yet), so if you see any ways to standardise it that is
> > great.
>
> I've made a few more changes. Could you please give them a try to make
> sure I didn't broke anything? :p
>
> I've moved the code that was common between read and info into a
> shared private function, checks after fseek and fopen, everything in
> one function (I see no need to have subfunctions since they are only
> called once anyway), replaced a long if block for a switch block,
> changed many checks for simpler, used dir to check the file size
> (rather than the low level fseek and ftell), and a lot of whitespace
> to make it pretty and extended documentation. I tried to make the code
> run in matlab (though I'd guess a matlab user would actually just use
> the matlab function).
>
> Here's the files:
> https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/private/analyze75filename.m
> https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/analyze75info.m
> https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/analyze75read.m
>
> > About the input checks - I'd aimed to make it behave as Matlab does for all
> > acceptable inputs. But for the cases where Matlab throws an error, this
> > function generally asks the user what to do instead. But I'm happy for this
> > to be simplified or left for other functions which call these.
>
> Done.
>
> Carnë

Changes look good.  Few minor edits to analyze75filename.m (see below), but otherwise everything works well.  Thanks for the help tidying it up!

-  function filename = analyze75check (filename)
+  function filename = analyze75filename (filename)

-  elseif (~exist (filename,'file')
+  elseif (~exist (filename,'file'))
 
-  error ('analyze75info: no file `%s''', filename)
+  error ('analyze75: no file `%s''', filename)

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev

analyze75filename.m (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

Carnë Draug-2
On 26 September 2012 11:10, adam aitkenhead <[hidden email]> wrote:

>> From: [hidden email]
>> Date: Tue, 25 Sep 2012 19:34:15 +0200
>> Subject: Re: [OctDev] Analyze75
>> To: [hidden email]
>> CC: [hidden email]
>>
>> hey, please avoid top posting. Reply at the bottom
>>
>> On 25 September 2012 17:07, adam aitkenhead <[hidden email]>
>> wrote:
>> >> From: [hidden email]
>> >> Date: Tue, 25 Sep 2012 16:30:48 +0200
>> >
>> >> Another thing I noticed is the many ways it tries to handle invalid
>> >> filenames. While it looks kinda slick, I'm not sure it's a good idea
>> >> to have the function doing it, it should be done by the script that
>> >> calls it. When you give wrong arguments to a function, trying to guess
>> >> what is right can lead to weird results. Or does matlab also behaves
>> >> that way (it's not documented at least). My suggestion would be to
>> >> maybe display the file selection if no argument is given but return an
>> >> error if the filenames are not valid files. What do you think?
>> >>
>> >
>> > All sounds good to me. Makes sense to keep to the standard way of doing
>> > things to make it behave as others might expect. I'm not all that
>> > familiar
>> > with the standard (yet), so if you see any ways to standardise it that
>> > is
>> > great.
>>
>> I've made a few more changes. Could you please give them a try to make
>> sure I didn't broke anything? :p
>>
>> I've moved the code that was common between read and info into a
>> shared private function, checks after fseek and fopen, everything in
>> one function (I see no need to have subfunctions since they are only
>> called once anyway), replaced a long if block for a switch block,
>> changed many checks for simpler, used dir to check the file size
>> (rather than the low level fseek and ftell), and a lot of whitespace
>> to make it pretty and extended documentation. I tried to make the code
>> run in matlab (though I'd guess a matlab user would actually just use
>> the matlab function).
>>
>> Here's the files:
>>
>> https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/private/analyze75filename.m
>>
>> https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/analyze75info.m
>>
>> https://sourceforge.net/p/octave/code/11114/tree//trunk/octave-forge/main/image/inst/analyze75read.m
>>
>> > About the input checks - I'd aimed to make it behave as Matlab does for
>> > all
>> > acceptable inputs. But for the cases where Matlab throws an error, this
>> > function generally asks the user what to do instead. But I'm happy for
>> > this
>> > to be simplified or left for other functions which call these.
>>
>> Done.
>>
>> Carnë
>
> Changes look good.  Few minor edits to analyze75filename.m (see below), but
> otherwise everything works well.  Thanks for the help tidying it up!
>
> -  function filename = analyze75check (filename)
> +  function filename = analyze75filename (filename)
>
> -  elseif (~exist (filename,'file')
> +  elseif (~exist (filename,'file'))
>
> -  error ('analyze75info: no file `%s''', filename)
> +  error ('analyze75: no file `%s''', filename)

Thanks. Fixed

------------------------------------------------------------------------------
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

adama
Hi Carnë,

I've just submitted analyze75write.m to SVN.

Adam

------------------------------------------------------------------------------
Got visibility?
Most devs has no idea what their production app looks like.
Find out how fast your code is with AppDynamics Lite.
http://ad.doubleclick.net/clk;262219671;13503038;y?
http://info.appdynamics.com/FreeJavaPerformanceDownload.html
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

Carnë Draug-2
On 1 October 2012 22:30, adam aitkenhead <[hidden email]> wrote:
> I've just submitted analyze75write.m to SVN.

Nice, thank you. The code looks nice. I have added the function to the
INDEX and NEWS file, and to the see also of the other functions.

Some tips, you don't need to check the value returned by isfield. If
there is a field with that name, it will return true so "if (isfield
(x, name))" is enough, no need to "if (isfield (x, name) == 1).

It would be good practice to also have analyze75filename deal with the
filename part, just as it's doing for the other 2 functions. I'd guess
analyze75filename would take an extra argument ("write", "info" or
"read") for the part of checking if the file already exists. This
would also allow to make it return the right error message with:

      error ('analyze75%s: error message.', analyze75function)

Carnë

------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
Reply | Threaded
Open this post in threaded view
|

Re: Analyze75

adama
> From: [hidden email]

> Date: Tue, 2 Oct 2012 09:33:17 +0200
> Subject: Re: [OctDev] Analyze75
> To: [hidden email]
> CC: [hidden email]
>
> On 1 October 2012 22:30, adam aitkenhead <[hidden email]> wrote:
> > I've just submitted analyze75write.m to SVN.
>
> Nice, thank you. The code looks nice. I have added the function to the
> INDEX and NEWS file, and to the see also of the other functions.
>
> Some tips, you don't need to check the value returned by isfield. If
> there is a field with that name, it will return true so "if (isfield
> (x, name))" is enough, no need to "if (isfield (x, name) == 1).
>
> It would be good practice to also have analyze75filename deal with the
> filename part, just as it's doing for the other 2 functions. I'd guess
> analyze75filename would take an extra argument ("write", "info" or
> "read") for the part of checking if the file already exists. This
> would also allow to make it return the right error message with:
>
> error ('analyze75%s: error message.', analyze75function)
>
> Carnë

Those suggestions sound good, I can make those changes although I probably won't have time before the weekend.  If you're ready for the release of the image package then don't wait, as it won't affect functionality.
 
Thanks,
Adam

------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev
_______________________________________________
Octave-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/octave-dev
12