patchGNU Octave - Patches: patch #9057, Octave Mapping package: majaxis...

 
 

patch #9057: Octave Mapping package: majaxis and minaxis

Submitter:  None
Submitted:  Sun 24 Jul 2016 07:15:26 PM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Originator Email:  -email is unavailable-
Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 15 Apr 2018 05:28:14 PM UTC, comment #7: 

closed, see patch #9566

Philip Nienhuis <philipnienhuis>
Group Member
Sat 20 Aug 2016 09:33:36 PM UTC, comment #6: 

Sorry for a delay (vacation and other priorities afterwards).

We try to keep up some quality standards. There is no reason to accept code that does not meet our minimum quality expectations yet, unless someone really wants it in and then he/she is expected to fix it first.

>> deg2rad (R)

error: 'R' undefined near line 1 column 10
===> What else would you expect if R has not been initialized? That is no deg2rad deficiency but a user error.
Now look here:

>> R = 'a'
>> deg2rad (R)

error: deg2rad: DEG must be a floating point class (double or single)
error: called from
    deg2rad at line 49 column 5
===> so there sure is error checking in deg2rad.

Yes I helped you once with comment #4, but I simply have no time to do that for every little aspect.
Please try to conform to our code standards and explore yourself what code should ideally look like - examples galore in the octave code base. Please do not try to lure us into accepting code that needs significant after-care.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 26 Jul 2016 06:00:32 AM UTC, comment #5: 

help majaxis works for me. Please show your results.

What is wrong with my style is spaces. Comment #4 is nicer way to show instead of looking at link. 
 
distance.m does not follow this and is approved:
function [dist,az] = distance(varargin) 

No error check in deg2rad and is approved:
deg2rad(R)
error: 'R' undefined near line 1 column 9
error: evaluating argument list element number 1

Anonymous
Mon 25 Jul 2016 07:37:25 AM UTC, comment #4: 

<savannah markup bit me again>

<cont. from comment #3>


function a=majaxis(b, e)
  ## The equation is b=a*√(1-e^2)
  ## rearranging a=b/√(1-e^2)
  a = b / sqrt(1 - e ^ 2);
  endfunction


should rather look like

function a = majaxis (b, e)

  ## The equation is b=a*√(1-e^2)
  ## rearranging a=b/√(1-e^2)
  a = b / sqrt (1 - e ^ 2);

endfunction



Also in the texinfo help text, spaces Octave's coding style is appreciated for better readability.

Immediately after the deftypefn line there should be a on-line brief description of what the function is used for - one sees that when typing "help majaxis".

In addition error checks are needed. What will happen if b = 0? b and/or e < 1, or non-numeric?

And of course tests are needed.

BTW are you the same person that also contributed patches 9051 and 9053?

Philip Nienhuis <philipnienhuis>
Group Member
Mon 25 Jul 2016 07:27:55 AM UTC, comment #3: 

Well, most of what you provided looks good, but there are things like this:

## Copyright (C) 2016

that rather should have a name:

## Copyright (2016) <your name>

and:

function a=majaxis(b, e)
  ## The equation is b=a*√(1-e^2)
  ## rearranging a=b/√(1-e^2)
  a = b / sqrt(1 - e ^ 2);
  endfunction
-verbatim

should look like:
+verbatim+
function a = majaxis (b, e)

  ## The equation is b=a*√(1-e^2)
  ## rearranging a=b/√(1-e^2)
  a = b / sqrt (1 - e ^ 2);

endfunction


and immediately after the deftypefn line there should be a concise one-liner explaining what the function does w/o going into details. If on types:
help majaxis
that line is shown.

Also in the texinfo help text we'd like to have spaces around "=" to improve readability. In comments that is less relevant although readability is also a concern there.

There's a tiny section on the wiki about coding style, see:
http://wiki.octave.org/Octave_style_guide, but there must be more.
Or study core Octave functions to see the coding style.

Another important issue is error checks.
What happens if someone inputs e < 1? b < 1? b = 0? b and/or e non-numeric?

Philip Nienhuis <philipnienhuis>
Group Member
Sun 24 Jul 2016 09:08:02 PM UTC, comment #2: 

I do not understand what is wrong with the style. Please provide examples.
 

 

Anonymous
Sun 24 Jul 2016 07:47:57 PM UTC, comment #1: 

Thanks for this contribution.

Do I conclude correctly you do not want your name mentioned? (no name in copyright string?
If you do however,   please supply your name (email address isn't required).

As to some fine-tuning of coding style, please see comment no. 2 in patch #9051. Also some tests are required.

If you can polish that up I think this contribution can be incorporated in a new mapping package release in the 2nd half of August.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 24 Jul 2016 07:15:26 PM UTC, original submission:  

Solves for the semimajor axis and semiminor axis

Anonymous

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #38007:  majaxis.m added by None (1KiB - application/vnd.wolfram.mathematica.package)
file #38008:  minaxis.m added by None (1KiB - application/vnd.wolfram.mathematica.package)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by philipnienhuis
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-04-15 philipnienhuis Open/ClosedOpen Closed
    2016-09-03 philipnienhuis Carbon-Copy- Added philipnienhuis
    2016-07-24 philipnienhuis CategoryNone Forge : new function
    2016-07-24 None Attached File- Added majaxis.m, #38007
        Attached File- Added minaxis.m, #38008

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code