patchGNU Octave - Patches: patch #8872, add isocaps

 
 

patch #8872: add isocaps

Submitter:  Markus Mützel <mmuetzel>
Submitted:  Tue 19 Jan 2016 04:32:59 PM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 13 Aug 2016 01:52:17 PM UTC, comment #22: 

I cleaned up and pushed the patch for isocaps here (http://hg.savannah.gnu.org/hgweb/octave/rev/3fe6663808cc).  Thanks, Markus, for all the work on the volume visualization functions.  The 4.2 release is starting to look very good.

Rik <rik5>
Group administrator
Wed 10 Aug 2016 03:47:51 PM UTC, comment #21: 

Rik, thank you, for pushing many of my patches. Attached you'll find an updated changeset which hopefully follows Octave's coding style.
I converted the tests that opened a figure into a demo and added also other new functions that have been pushed recently to NEWS. I hope this is OK.

(file #38168)

Markus Mützel <mmuetzel>
Group administrator
Wed 10 Aug 2016 03:31:45 AM UTC, comment #20: 

@Markus: Could you refresh the isocaps patch after looking at one of the reworked functions like reducepatch.m?

The patch is not applying cleanly.  After fixing the reject file from diff, I ran 'test isocaps'.  All 32 tests pass, but it also opened a graphics window which it shouldn't have.

Rik <rik5>
Group administrator
Wed 20 Jul 2016 08:04:58 PM UTC, comment #19: 

Tested demo on Windows; works fine AFAICS. A green-yellow sphere clipped at 3 sides (dark blue-green).

"test isocaps" will have to wait - my cross-build (and as it seems, the actual dist target on Linux on which it is based) seem to be fubarred (missing macros.texi).

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jul 2016 05:15:36 PM UTC, comment #18: 

Markus,
my test (cross-)build dates from last Friday so no I didn't try the latest patch.
I noted that entering "v" instead of verbose made the demo run fine.

I plan to make a new patch build tonight and I'm just scouring the patch tracker for updated patches :-)

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jul 2016 03:37:48 PM UTC, comment #17: 

Philip, it looks like the patch did not apply completely. Lines 587&588 of it should add the case to isosurface.m that handles the empty string (that is ignoring it).

Did you apply the latest changesets (the ones I uploaded yesterday) in the order given in comment #6?

Markus Mützel <mmuetzel>
Group administrator
Tue 19 Jul 2016 03:25:30 PM UTC, comment #16: 

The demo doesn't work.
Stepping manually through the demo in debug mode:

:
>> isonormals (x, y, z, val, p_iso)

>> fvc_xmin = isocaps (x, y, z, val, iso, "xmin", "b")

error: isosurface: parameter '' not supported
error: called from
    isosurface>__get_check_isosurface_args__ at line 225 column 9
    isosurface at line 148 column 41
    isocaps>__get_isocaps_patches__ at line 328 column 19
    isocaps at line 140 column 21
>> fvc_xmin = isocaps (x, y, z, val, iso, "xmin", "b")
stopped in D:\Octave\Octave-4.1.0+VEp_w64_20160714\share\octave\4.1.0+\m\plot\draw
\isocaps.m at line 140
140:     [faces, vertices] = __get_isocaps_patches__ (x, y, z, data, iso, pad_val,
 ...
debug>
debug> verbose
verbose =
debug> varargin{i_arg}
ans =
debug> uint8 (varargin{i_arg})
ans = [](0x0)


It seems the "verbose" argument hasn't been initialized in isocaps.m or its subfunctions.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jul 2016 03:10:31 PM UTC, comment #15: 

About material and isocaps references:
Don't search any further, the files were not included in my build, I suppose due to the mercurial issues with umlauts.
So the cause of those texi issues I hit is clear now.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jul 2016 11:21:02 AM UTC, comment #14: 

I found the issue that led to a problem when cross-referencing "reducepatch" (see comment #10) and updated the patch there.
I cannot duplicate the issue with cross-referencing "material" here. But will try to find out what is going wrong anyway.

Regarding the umlaut-u: It seems the headers of my generated patches have not been UTF-8 encoded. I hope future uploads will be ok.

Markus Mützel <mmuetzel>
Group administrator
Tue 19 Jul 2016 06:21:01 AM UTC, comment #13: 

BTW, in many of your patches I had to change "mützel" into "muetzel" to have mercurial accept them.
mercurial is at v. 3.1.1 on my Mageia-5 systems.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jul 2016 06:06:31 AM UTC, comment #12: 

Markus,

"hassle" - please don't exaggerate :-) this is a development tree, there's always a slight chance to stumble upon inconveniences :-)

Anyway: checking my hg log and local commits I found I made the following changes to get the build going:

- isosurface.m: removed "reducepatch"from seealso line
- light.m: removed "material" from seealso line
- reducepatch.m: removed "isocaps" from seealso line. In addition this patch had CR-LF endings, but that is probably a local issue (read: probably my own fault)

Philip Nienhuis <philipnienhuis>
Group Member
Mon 18 Jul 2016 06:01:10 PM UTC, comment #11: 

The attached patch should apply cleanly on top of file #37938 in patch #9013 "isocaps".

(file #37939)

Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jul 2016 05:16:10 PM UTC, comment #10: 

Sorry, Philip for causing such a hassle.
I think I found the issue that caused the problems for you. There is a trailing space at the end of that line in "reducepatch.m":

## -*- texinfo -*-


I will wait for the build to finish and upload patches that should apply cleanly on top of each other in the order given in comment #6 (short of #5 "camlight" which is none of "my" patches).

Markus Mützel <mmuetzel>
Group administrator
Mon 18 Jul 2016 03:13:02 PM UTC, comment #9: 

No sorry, I wiped the build log (actually the entire build dir).
IIRC it was one of the texi processing tools complaining about a mangled, or if you want: decorated, isocaps identifier. The error mentioned "possibly also mentioned in seealso" or s/th like that.
I just wiped those fucntion names from the @seealso refs and then the build went through.

Indeed, interdependencies between your contributions cannot be ruled out - you are very productive (thanks!). In one of your other patch reports I suggested to provide a listing or so mentioning in which order your contributions had best be pushed.

I have to see when I can have another go at making a patch build from a clean default tree. If not it'll be until after mid-August.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 16 Jul 2016 05:29:48 PM UTC, comment #8: 

Philip, I just tried locally with this patch here and the one for material. I cannot see any errors while building the docs.
Do you still have access to the build log with the complete error message? The error you are seeing might be due to some unresolved conflicts from applying different conflicting patches...

Markus Mützel <mmuetzel>
Group administrator
Sat 16 Jul 2016 10:39:50 AM UTC, comment #7: 

@Markus
There were some troubles when building docs with several of your patches. I think that is because some of your functions go to a private subdir; that implies that seealso references cannot point to those functions.
"isocaps" is one of them, but the build process also complained about "material" and another one I forgot.

Philip Nienhuis <philipnienhuis>
Group Member
Fri 08 Jul 2016 03:45:31 PM UTC, comment #6: 

Thank you, Lachlan, for reviewing that patch.

You did raise some valid points. All of your concerns should be addressed in the attached changeset.

I tried to improve the docstring but this is hard for me because English is not my native language. All I could come up with is:
"This function places caps at open ends of isosurfaces."
which makes the first two sentences of the docsting a little bit redundant.

Could you please run the demo (which I added in favor of the poor example)? That should make clearer what this function actually does.
Maybe, you could find some better word for "end-caps" or describe better what is ment.

Wrt the other patches: As much as I would like to get all of them into 4.2, I see that this is probably hard taking into acount that there is only one week left before feature freeze.

Since you asked, here is a list with some of the patches starting with the easier/smaller (but imho nevertheless useful) ones which I think should reviewed first if time permits:
1- patch #9040 "lighting": That is mostly the same as "shading" with added changes from bug #48392.
2- patch #8864 "smooth3": Mainly just a wrapper for convn.
3- patch #8856 "reducevolume": No magic at all. Most complicated part was to get the input right.
4- patch #8912 "reducepatch": A little bit more complex but something I use quite frequently.
5- patch #9014 "camlight": None of "my" patches but something I'd very much like to see included.
6- patch #9013 "material": Something I added "for the sake of completeness". Should not contain anything harmful but quite low on my list.
7- This patch #8872 "isocaps" since you already started reviewing.

Other patches need probably more time to get pushable and won't be ready for 4.2.0.

Another piece of code that I would find helpful is Pantxo's file #33886 from patch #11311.

This list is probably quite excessive. If we could get at least some of these patches into 4.2, it would be really nice.

The latest changeset in patch #8943 ("light") is mainly error fixes and can probably still be applied after feature freeze. The same holds maybe for most (or some) part of patch #8901 (the infamous "fminsearch" one). But the latter with lower priority.

(file #37773)

Markus Mützel <mmuetzel>
Group administrator
Thu 07 Jul 2016 02:38:02 AM UTC, comment #5: 

Thanks, Markus.

I forgot to mention that we also need to change all docstrings to replace {Function file} by {}.

Other comments are:
1. Are you sure which_caps can only be a/above or b/before?  If not, it may be better to make "b", "before" a separate case and have "otherwise" throw an error.

2. I think in-line comments should start with a single "#".

3. It is expensive to building faces and vertices one element at a time like in the 'if (strcmpi (which_plane, "all"))' since it requires a copy each time the matrix grows.  It would be faster to allocate f1, f2, ..., f5, and then form [faces; f1; f2; f3; f4; f5].  Similarly, it is faster to build vertices_grid from row 3 to row 1, or to preallocate it.

4. Several of the lines are too long.

5. It would be good it the docstring didn't assume that the reader knows what end-caps are.

6. I'm not sure, but I think you can replace


@table @asis
@item @code{above}


by


@table @code
@item above


If so, the latter probably expresses your intent better.

7. _get_isocaps_patches_ seems quite repetitive, which makes maintenance hard.  Is it possible to use the switch statement to choose a small number of parameters (like which dimension to use), and then have a single isosurface command?  That may end up being less clear (which is also a maintenance issue).

Hmm... You have submitted lots of patches (thank you!) and it will take a long time to review them all.  Would you like to prioritise your patches, with the ones you think should be reviewed first?

Lachlan Andrew <lachlan>
Wed 06 Jul 2016 07:38:34 PM UTC, comment #4: 

Lachlan, attached please find a complete changeset.
Since the latest version of "__calc_isovalue_from_data__" was already pushed in 634fbedbfb5b for bug #46946, it is no longer included here.

(file #37737)

Markus Mützel <mmuetzel>
Group administrator
Wed 06 Jul 2016 11:15:29 AM UTC, comment #3: 

Markus, if you would like this to be considered for 4.2.0, could you please create a changeset from this, with the changes to modules.mk etc.?

Lachlan Andrew <lachlan>
Thu 21 Jan 2016 01:55:17 PM UTC, comment #2: 

I implemented the one- and four-input-arg options and updated the help text and the tests accordingly.

There are many different ways to get a (probably good) value for an isosurface from 3d data. "__calc_isovalue_from_data__.m" returns the same value that Matlab would use taking the same approach.

This function should go to the "private" folder where "isosurface.m" is and "isocaps.m" will be stored.
Maybe "isosurface.m" could use the same function to work with one and four input arguments (like in Matlab).

I do not know the naming convention for functions in private folders. So please tell me if I should rename it.

As to isosurface, I filed bug #46946 describing the issue with "verbose" and some other deviations from Matlab syntax.

(file #36110, file #36111)

Markus Mützel <mmuetzel>
Group administrator
Tue 19 Jan 2016 08:47:00 PM UTC, comment #1: 

If you can implement the one-input-arg option and document it (texinfo header) that would be great, but not quite obligatory.

The issue with undocumented Matlab is that TMW can change it any moment and Octave would have to follow. But maybe that isn't such a big deal in this case. There's lots of undocumented-Matlab-behavior implemented in core Octave anyway (and some devs do feel a little itchy about that).

As to isosurface: indeed, "verbose" seems to do nothing. Can you file a bug report about that, after comparing with Matlab (with the same data set) to verify that Octave behaves differently?

Other than that, following the first example in "help isosurface" just works fine here.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 19 Jan 2016 04:32:59 PM UTC, original submission:  

This is an implementation of the function 'isocaps'.

While testing, I found two minor issues:
Matlab allows calling this function with only one input argument. In this case, it calculates a value for the iso-surface from the 3d data. The same goes for the function 'isosurface'. However, this is nowhere mentioned in the official documentation for none of the two functions (at least, I have found no documentation on this).
Should I try to implement it anyway? As of now, calling 'isocaps' or 'isosurface' in Octave with one input argument throws an error.

The second issue is in 'isosurface': Even if the help text for isosurface states that it can be called with the additional input argument "verbose", it does not seem to work. According to the help text, this should work:
isosurface (rand (12, 12, 12), .5, "verbose")
However, it throws: "error: COLORS must be a matrix of dim 3 and of same size as C"
As of now, 'isocaps' accepts "verbose" as an arguments but does not pass it on to isosurface (effectively ignoring it).
Is this behaviour OK?

Markus Mützel <mmuetzel>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #38168:  patch8872_isocaps_v3.patch added by mmuetzel (22KiB - text/x-patch)
file #37939:  isocaps_v3.patch added by mmuetzel (21KiB - text/x-diff)
file #37773:  isocaps_v2.patch added by mmuetzel (21KiB - text/x-diff)
file #37737:  isocaps.patch added by mmuetzel (20KiB - text/x-diff)
file #36110:  isocaps.m added by mmuetzel (18KiB - text/plain)
file #36093:  isocaps.m added by mmuetzel (16KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by mmuetzel (Submitted the item)
  •  

    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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-08-13 rik5 StatusNone Done
        Open/ClosedOpen Closed
    2016-08-10 mmuetzel Attached File- Added patch8872_isocaps_v3.patch, #38168
    2016-07-18 mmuetzel Attached File- Added isocaps_v3.patch, #37939
        Carbon-Copy- Added philipnienhuis
    2016-07-08 mmuetzel Attached File- Added isocaps_v2.patch, #37773
    2016-07-06 mmuetzel Attached File- Added isocaps.patch, #37737
    2016-01-21 mmuetzel Attached File- Added isocaps.m, #36110
        Attached File- Added _calc_isovalue_from_data_.m, #36111
    2016-01-19 mmuetzel Attached File- Added isocaps.m, #36093

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code