-
-
Notifications
You must be signed in to change notification settings - Fork 62
Implement ContourPlot3D #1592
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
base: sphericalplot3d
Are you sure you want to change the base?
Implement ContourPlot3D #1592
Conversation
bdlucas1
commented
Jan 3, 2026
rocky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please follow the existing documenting conventions, where at least one user-facing example is given. Examples with commentary would be nice. And also add a link to wikipedia for the concept at the beginning when that is applicable as it often is here. Thanks.
| <dt>'ContourPlot3D'[$f(x,y,z)$, {$x$, $x_{min}$, $x_{max}$}, {$y$, $y_{min}$, $y_{max}$, {$y$, $y_{min}$, $y_{max}$}] | ||
| <dd>creates a three-dimensional contour plot of $f(x,y,z)$ over the specified region on $x$, $y$, and $z$. | ||
| >> ContourPlot3D[x ^ 2 + y ^ 2 - z ^ 2, {x, -1, 1}, {y, -1, 1}, {z, -1, 1}] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples don't belong inside <dl> lists. Look again at the docs to see how this is typically done.
If it is not fixed in a day or so, I'll put in a PR to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Co-authored-by: R. Bernstein <[email protected]>
rocky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I'd like to understand the code in more detail. Don't have time right now, though. Possibly some other time.
Overall, this is cool stuff. I am concerned about (inadvertently) making other existing stuff non-functional.
| def equations_to_contours(plot_options): | ||
| """ | ||
| For contour plots, convert functions of the form f==g to f-g, | ||
| and adjust contours ot [0] and disable background |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ot? is to? Or something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
I am too; that's why I put a lot of effort into putting tests in place for the existing plotting function to reduce that risk. What I think those tests tell us is that every example in the doc still generates the same output, both Graphics* and SVG. And in addition to testing at that level, I have tests in m3d where I generate PNGs from the SVGs, and from the Graphics* (via m3d rendering) and compare them automatically for changes and manually for mismatches. |
The expanded testing is appreciated, although it probably will take me a little while to learn and get used to. It's not the stuff that we currently test that is a worry, it is the stuff that we don't test. Like documentation. I think @mmatera has some tests here, but stuff like adding a like to a relevant wiki page and making regular the formatting is not something that is easily done. |
That is why I have been hesitant to touch the docstrings for builtins etc., and was happy with the offer of help from @mmatera on the first PR I submitted without examples to add those later. I thought process we were on was to accumulate them on #1586 to be done later in a batch (whether by me at the end of the day, or by someone more experienced with the arcane system). From my perspective it would have been far more efficient, and would likely have produced a better result, to have done all those in a batch in a single PR. Spending the morning struggling with an arcane markup in three inter-dependent PRs and dealing with things like Windows charsets was not how I had envisioned spending my Saturday morning. |
@bdlucas1, now I am working on another part of the code. When I finish (at least a part) of what I am doing, the next is to go over the new symbols to add the missing documentation. |
|
For my part, I am happy if @bdlucas1 completes the implementation of these graphics routines and tests. Adding (basic) docs is for me something that is not too hard to do before the release. |
If you want to add builtins, you should be taking more responsibility for understanding how builtins are written and following the conventions we use. The first or first couple of builtins, it is understandable that something might slip through.
From my perspective, things like this slip until it becomes a mess. Right before release, there is this huge chore of trying to get things regularized and sometimes unbroken. If I recall correctly, the more important part was to get a rendering API and architecture in place. |
Thanks @mmatera. I've added some suggested examples to #1586 in case it helps. Let me know if there's anything else I can do to help. |
|
I started with the documentation. The problem I find is that symbols like |
This was exactly the kind of thing I wanted to detect and avoid. From the project point of view, it is better to figure this out earlier than later. |
The status of these feature is that they generate Graphics* output that contains GraphicsComplex and NumericArray, which will not be understood by most current consumers (aside from the experimental front-end m3d, which I use as a proof-of-concept and test vehicle). That was the reason for my reference in #1586 to a dependency on #1589, which is one way of enabling current consumers to be able to consume the output of the vectorized plotting functions.
The intention of the variable is that it be dynamic, e.g. so we can run both vectorized and non-vectorized plotting tests. I haven't looked, but I suspect that's the reason for the test failures in your PR. Having this be dynamic has also been a great help in development work (my initial implementation was static and it proved to be awkward). In the examples that I added this morning, I just showed the unevaluated expression as the output, which is what the doctests want to see. This of course isn't helpful to a user. I'm thinking that given the incompleteness (from an end-to-end system) point of view of the vectorized plotting features, it might be reasonable to leave them undocumented until we have a more complete picture. Or in the alternative document them but clearly mark them as "preview" or "incomplete" or "coming soon". |
The incompleteness was recently created. It's going to have to be fixed before we can release. And it would be better to fix it now. If you don't want to fix the incompleteness caused by these recent features, that's okay. I'll do it when I can. But then hold off on doing more, please. |
One other thing regarding incompleteness. The reality, right now, is that we probably have to provide fallback ability to plot for plots where there are scoped variables and procedural-type statements (until we get compilation under control, and that's a big topic). |