bugGNU Octave - Bugs: bug #57471, streamtube.m should use surface...

 
 

bug #57471: streamtube.m should use surface object rather than patch object

Submitter:  Rik <rik5>
Submitted:  Mon 23 Dec 2019 03:54:08 PM UTC
   
 
Category:  Octave Function Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 01 Mar 2020 12:52:03 PM UTC, comment #21: 

I slightly reworded the NEWS file entry to be closer to what Rik suggested in comment #17 and pushed in to stable here:
http://hg.savannah.gnu.org/hgweb/octave/rev/4f1d1b1d4afc

I think this is all that needs to be done for this report.
Closing as fixed.

Markus Mützel <mmuetzel>
Group administrator
Fri 28 Feb 2020 07:14:25 AM UTC, comment #20: 

The NEWS-file change

(file #48515)

Hg200 <hg200>
Wed 26 Feb 2020 08:01:58 AM UTC, comment #19: 

I won't be able to push anything until Saturday or Sunday. I can make that change then if no one else beats me to it earlier.

Markus Mützel <mmuetzel>
Group administrator
Tue 25 Feb 2020 08:20:18 PM UTC, comment #18: 

Hmm - any grammar corrections? Markus, can you push or shall i upload a patch here?


- The function `streamtube` plots tubes along streamlines which are
  scaled by the vector field divergence, as Matlab does. The
  Octave-only extension `ostreamtube` can be used to visualize
  the flow expansion- and contraction of the vector field due to the
  local crossflow divergence.

Hg200 <hg200>
Tue 25 Feb 2020 07:47:21 PM UTC, comment #17: 

Could you add a note to the NEWS file in the "Matlab compatibility" section noting streamtube, ostreamtube, and why one might use one over the other?  I think something like "streamtube is Matlab-compatible and produces a divergence plot of the vector field.  ostreamtube is an Octave-only extension which produces a classical plot"  Or something like that, this is not my mathematical field of expertise.

Rik <rik5>
Group administrator
Tue 25 Feb 2020 06:32:00 PM UTC, comment #16: 
Markus Mützel <mmuetzel>
Group administrator
Sun 23 Feb 2020 07:08:20 PM UTC, comment #15: 

I think it is worth not confusing 6.1 users with a streamtube which is not Matlab-compatible, but becomes Matlab-compatible in the 7.1 release.  Hence, go ahead and push this to stable.

Rik <rik5>
Group administrator
Sun 23 Feb 2020 10:55:24 AM UTC, comment #14: 

Usually, there are only two branches in the Octave repository.
The default branch is for developing the next Octave version. Until a few days ago that was Octave 6.0.
The stable branch holds the current stable release. Until a few days ago that was Octave 5.2.
Version with the minor version "0" are for development only. The first released version will have the minor version "1". Subsequent bug fix release will have the minor version "2", "3", ...

A few days ago, default was merged to stable in preparation of the upcoming release of Octave 6.1. Since then, default is Octave 7 and stable is Octave 6.

"streamtube" was initially pushed to default (when it was still Octave 6.0). Since the merge, it is also in stable.

Usually, changes on the stable branch should only include bug fixes or documentation improvements.
I'm unsure if renaming and adding functions still counts as fixing bugs. One might argue that it fixes a Matlab compatibility bug. But "ostreamtube" isn't a Matlab function. Hence, my uncertainty.

Markus Mützel <mmuetzel>
Group administrator
Sun 23 Feb 2020 10:05:16 AM UTC, comment #13: 

Thanks for the twist improvement. "bug57471_ostreamtube_v2.patch" + "bug57471_streamtube_v2.patch" are a bit better and I also recognize the missing reference in stream3 + streamline. I made a fresh build and ran some streamline validation scripts. No findings.

I have not tested on the current stable branch. But from my side I agree to do the merge into stable. I don't expect any technical trouble. The only point I can think of is whether everyone is now okay with the doc sections (translation). But to be sure, a complete check by a native speaker is necessary.

BTW I was always afraid to ask: Is it correct that the standard procedure is to push patches and new functions on the default branch and after a while (before a release) the stable branch is overwritten by the default branch? Is there a possibility to tag a commit so that it will be merged to stable only in a major release (e.g. 7.0 instead 6.1)?

Hg200 <hg200>
Sat 22 Feb 2020 02:21:58 PM UTC, comment #12: 

I find it more logical when it comes to preserving file history to first rename "streamtube" to "ostreamtube" and then add a Matlab compatible "streamtube" function (see the attached changesets: "bug57471_ostreamtube_v2.patch" before "bug57471_streamtube_v2.patch").

I was about to push them on default. But would it be OK to push them on stable? The advantage being that we would have a Matlab compatible version of the function from the get-go and won't need to deal with compatibility issues between versions of Octave.

(file #48471, file #48472)

Markus Mützel <mmuetzel>
Group administrator
Wed 19 Feb 2020 09:27:38 PM UTC, comment #11: 

Thanks for reviewing the code.

From the IRC octave channel i got the proposal to either speak of

i.) "vertex array"
ii.) "array of vertices"

The term "polygon point" does not seem to be a good choice, since it can be understood as "a point inside a polygon" rather than an intersection between two edges.

Two patches for pushing into the repo:

patch_streamtube_divergence_plot_2020-02-19.diff
--> replaces "classic streamtube" by matlab compatible "tube scaled by divergence". you may find some "vertices-polishing" and a fix of the test section.

patch_ostreamtube_2020-02-19.diff
--> keep "classic streamtube plot" in ostreamtube.m. no technical changes. polishing + streamtube2ostreamtube replacement.


(file #48453, file #48454)

Hg200 <hg200>
Sun 16 Feb 2020 05:51:09 PM UTC, comment #10: 

English is not a very logical language, partially because it has borrowed frequently from other languages.  The singular is "vertex", the plural is "vertices".  The odd nature of the plural (not just adding an 's' at the end as is customary) is because this word came to English from the dead language Latin and the rules for forming plurals in that language were different.

Rik <rik5>
Group administrator
Sun 16 Feb 2020 09:47:55 AM UTC, comment #9: 

I also like that idea. And your patch looks good to me.

Could you please split the patch into two parts? One that renames the current "streamtube" to "ostreamtube" and a second one that adds the Matlab compatible "streamtube".

Also, I'm not sure if "vertice" is an English word. Could you please replace it by "vertex" in the doc string?
Maybe it would be nice, too, if the doc strings referred to each other with @seealso.

Thanks again for your very valuable contributions.

Markus Mützel <mmuetzel>
Group administrator
Sun 09 Feb 2020 03:36:49 PM UTC, comment #8: 

Rik's suggestion is very good because if the two concepts live in two different files, it's easy to maintain and easy to implement. I have attached a patch that implements his proposal. This patch is intended for commenting and testing. It can be split further before pushing (if required).

To understand the two concepts please have a look at the attached example. Consider the following vector field, which only depends on the x-direction:


u = 2 + 8 * exp (-2.0*x.*x);
v = zeros (size (x));
w = zeros (size (x));


Properties: The divergence of the field is approximately zero at the x = +-2 positions. The divergence is minimum and negative at x = 0.5 and maximum and positive at x = -0.5. The speed (i.e. the norm of the velocity vectors) is greater than 0 at the positions x = +-2 and is highest at the position x = 0.

1.) Classic streamtube plot (right hand side):
Since the vector field depends only on the variable x, all streamlines are straight and parallel, so the solution is a straight tube with a constant diameter.

The colorbar shows the linear speed "spd" of the vector field along the streamline. It is highest at the position x = 0 where it becomes 10.

2.) The divergence plot (left hand side):
Since the tube radius must always be greater than or equal to zero, the divergence must be "auto-scaled" before plotting. At the location x = 0.5 the tube diameter is zero because the divergence has its lowest value (it is also negative). At the position x = -0.5 the pipe diameter becomes highest because the divergence is at its maximum. It is important to understand that a tube diameter greater than 0 can also correspond to a divergence that is zero or even negative, as is the case at position x = -2. So it's a bit difficult to interpret.

Coloring is done depending on the "height" (z-axis). So the colorbar is not meaningful any more.

---

However if we proceed as above i would not yet apply the compatibility changes mentioned in comment #2 into ostreamtube(). I would do that later, maybe when adding streamribbon (however i got stuck there but thats a different point). For now, I would just go with a straight copy without any technical change.

(file #48383, file #48384,

Hg200 <hg200>
Mon 03 Feb 2020 04:52:44 PM UTC, comment #7: 

I like your arguments for a different behavior from Matlab.  We do occasionally diverge.  The comet() plotting function, for example,  is not identical.

I think the cleanest way would be to have two functions.  The one named "streamtube.m" should probably strive for exact compatibility.  The second one could use the prefix "o" for Octave and be called "ostreamtube.m".  We have done this with strsplit() and ostrsplit().  In that case, Octave had an existing function called strsplit() and The Mathworks eventually added a function with that same name, but which had a different behavior.  This was tedious since we had a perfectly good function, and that's why we decided to just rename it rather than delete it.

If that does sound like too much work then I restate my opinion (only one voice) to implement the more mathematically interesting behavior (not a "divergence  plot").

Rik <rik5>
Group administrator
Sun 02 Feb 2020 11:34:44 PM UTC, comment #6: 

Pros and Cons are:

When I call "streamtube()", I expect a "streamtube" as described for example on Wikipedia, rather than a "divergence-plot". In addition, a classic "streamline" can be plot for all types of vector fields, whereas a "divergence-plot" will show numerical errors for those vector fields that are divergence free. For example this is the case for incompressible fluid flows which satisfy the continuity equation.

Examples for Matlab versus Octave Plots:

https://josoansi.de/octave_plot_compare/compare_plot_demos_08.html

"Demo 1" shows a vector field with constant divergence everywhere. Octave shows how the streamlines converge at the bottom. The "divergence-plot" only shows numerical errors, since the tube radius is scaled to the min and max of the divergence. "Demo 2" shows an electrostatic field caused by a point charge. This field is free of divergence, except at the singularity. A classic "streamtube" plot shows how the field converges. A "divergence-plot" is useless because the divergence is zero everywhere. Consequence: When switching the algorithm we have to remove all demos. I don't have any good examples to plot normalized divergence.

On the other hand, after further research, I found that the "tube filter" in Paraview allows plotting "tubes along a streamline" whereby the tube diameter can be "controlled by a scalar value". So the Matlab solution can also be found in some post-processing programs. I also feel compatibility is a primary goal for Octave.

In the end it is a niche function (I think nobody will use it at all ???) and I don't want to extend the discussion too much. I need some time, but I can upload here a patch that is Matlab compatible and that shows the normalized divergence. However i would prefer to push only one of the two solutions and not a mixed solution. It would be great if Philip can contribute with a third opinion. We can then still decide which algorithm is Octave's final choice and hopefully close the topic.

Hg200 <hg200>
Sun 02 Feb 2020 10:14:06 PM UTC, comment #5: 

I can help testing but then I'd need some test code & test data

Philip Nienhuis <philipnienhuis>
Group Member
Sun 02 Feb 2020 04:53:56 PM UTC, comment #4: 

Usually, we try to have one change per commit. So it would have been better if you provided two separate patches for the tube radius and the graphics object type.

I pushed your changes here anyway:
http://hg.savannah.gnu.org/hgweb/octave/rev/0cd5f632a4b0
http://hg.savannah.gnu.org/hgweb/octave/rev/465be7c652f1

Wrt the used algorithm: In general, different results in Matlab and Octave could be considered as a bug. But we have some areas where we try to be "better than Matlab".
You seem to have a better background in fluid dynamics: Does one  algorithm have an advantage over the other? Would it make sense to support both algorithms?

Markus Mützel <mmuetzel>
Group administrator
Mon 06 Jan 2020 04:33:42 PM UTC, comment #3: 

attachments for mentioned points 2-4.

(file #48171, file #48172)

Hg200 <hg200>
Wed 01 Jan 2020 01:36:00 PM UTC, comment #2: 

I did a detailed check because the function was more of a quick shot used for validation of other stream code. There are several issues.

1.) Matlab offers a second option for calling the function (unimplemented):


streamtube (verts, x, y, z, div)


Based on the current algorithm used, the vector field strength would have to be specified additionally. The reason is as follows: What you currently get in octave is an approximation of the formal definition of a streamtube. This is a set of streamlines that run through a closed contour (see e.g. Batchelor - Introduction to Fluid Dynamics). In Matlab, a tube is plot along a streamline and the tube diameter is modulated by the divergence. This approach is different.

Looking ahead, I see two options: Keep the code (show the flow expansion) and possibly add the missing interface, including the additional "speed" parameter. Or if Matlab compatibility rules, discard the code to continue with a new algorithm (= draw a tube scaled with a scalar). Please let me know your thoughts or let me know if further explanations are required.

2.) Continuing with the cleanup that started in f2b89a2e20b6
3.) Fix the plot method (surface instead of patch) and also fix an incorrect color bar setting
4.) If a singularity is hit prematurely the code must continue instead of exiting. The tube radius must also depend on the size of the plot object instead of the cell size.

For issues 2+3+4 i have already prepared different patches but depending on the decision on 1 they may become obsolet.

Hg200 <hg200>
Mon 23 Dec 2019 07:20:41 PM UTC, comment #1: 

I'll take care of it. if patch () is replaced by surface (), the vertice construction can be simplified. significant cpu runtime can be gained. in maybe a week i will complete with streamribbon. code is similar between functions. --> can take over code from streamribbon to create a patch for streamtube.

Hg200 <hg200>
Mon 23 Dec 2019 03:54:08 PM UTC, original submission:  

The underlying graphic object for streamtubes in Matlab is a surface object.  It shouldn't be that hard to use that object in Octave as well.

Rik <rik5>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #48515:  patch_NEWS.diff added by hg200 (982B - text/x-patch)
file #48471:  bug57471_ostreamtube_v2.patch added by mmuetzel (11KiB - application/octet-stream)
file #48472:  bug57471_streamtube_v2.patch added by mmuetzel (18KiB - application/octet-stream)
file #48384:  concept_comparison.m added by hg200 (1KiB - text/x-objcsrc)
file #48385:  comparison.png added by hg200 (90KiB - image/png)
file #48172:  patch2_06_01_2020_plot.diff added by hg200 (6KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by hg200 (Posted a comment)
  • -email is unavailable- added by rik5 (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 group members can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-03-01 mmuetzel StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-02-28 hg200 Attached File- Added patch_NEWS.diff, #48515
    2020-02-22 mmuetzel Attached File- Added bug57471_ostreamtube_v2.patch, #48471
        Attached File- Added bug57471_streamtube_v2.patch, #48472
    2020-02-19 hg200 Attached File- Added patch_ostreamtube_2020-02-19.diff, #48453
        Attached File- Added patch_streamtube_divergence_plot_2020-02-19.diff, #48454
    2020-02-09 hg200 Attached File- Added patch_streamtube_divergence_plot.diff, #48383
        Attached File- Added concept_comparison.m, #48384
        Attached File- Added comparison.png, #48385
    2020-02-02 mmuetzel StatusConfirmed Ready For Test
    2020-01-06 hg200 Attached File- Added patch1_06_01_2020_cleanup.diff, #48171
        Attached File- Added patch2_06_01_2020_plot.diff, #48172

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code