-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Addtional 3D plot options #57
Comments
Thanks for pointing out this issue. I agree with you that keeping adding optional parameters is not a very scalable solution. [b] seems like the right direction, by using some type constructor imo. As you mentioned, we need to find a good balance between flexibility and simplicity. I think this can be achieved actually in Owl. Ideally, every plot function should have:
By using a list of config parameters, we can increase the flexibility and also maintain the simplicity. It will also actually help in simplifying the current API. E.g., if we have a new type called type config =
| Color of int * int * int
| Linestyle of int
| Contour of bool
| ....
``
Then current `plot` function will become
```ocaml
val plot : ?h:handle -> ?config: config list -> dsmat -> dsmat -> unit which is much nicer. This requires some re-wrapping of the config types in PLPLOT and also changes the existing plot functions. But since there are not many plotting functions at the moment in Plot module. I think this should be OK. I guess this design can also satisfy what you need now, right? I am happy to accept your current solution, but in the long run, we need to change to the aforementioned design imo :) |
Yes, I can see why this makes sense.
Do you mean creating an OCaml type in Here are some "thinking out loud"-style thoughts about the general
|
This all sounds good to me. If there's a clear path at the moment for using the new type strategy, I could implement it in |
There are variations on 3D plots that Owl doesn't currently provide but that would be useful for me. For example, this plot was created by using
[PL_DRAW_LINEY]
as theopt
argument toplmesh
:foo.pdf
(The color variation in the plot is something I'll discuss in a different issue.)
I don't want to make Owl's plotting API overly complicated. Its simplicity is what makes it so nice to use. So I've thought about different ways to make
mesh
andsurf
more flexible. This is my current thinking:Ideally it's good to give users as much flexibility as possible with
mesh
andsurf
.(a) However, adding an optional argument for every option that can be passed (e.g.
PL_DRAW_LINEY
, etc. is confusing and would make the interface tomesh
andsurf
a mess.(b) The number of optional arguments could be reduced by adding only one optional argument that allows users to pass something like
Plplot.([ PL_DRAW_LINEXY; PL_MAG_COLOR; PL_MESH ])
. This is confusing, though, and interferes with the simplicity of the Owl interface.(c) Another option is to define new 3D plot options that use different plplot option lists. However, it's also important to avoid writing many different plot functions with a lot of duplicate code. (On the other hand, I do think that a little bit of duplicated code can be easier to understand rather than abstracting out too much of the code into common functions.)
So there's no ideal solution, other just than (d) not allowing more flexibility in 3D plotting.
My inclination at this point is use option (b) to add an optional first argument to
mesh
andsurf
. This argument allows passing a list of plplot options forplmesh
,plmeshc
, orplsurf3d
. It's not a nice interface, but then one can use this optional argument to define new 3D plotting functions as in (c).For example, this is the definition of the function
plots2d_in_3d
that I used to generate the PDF linked above:(I couldn't think of a better name for this function.) The full revised code for
mesh
that I used (andsurf
, too) is here: https://github.com/mars0i/owl/blob/master/lib/owl_plot.ml#L877What do you think? I didn't want to submit a PR before seeing what your thoughts about this were.
The text was updated successfully, but these errors were encountered: