question about the new adaptive sampling in plots

[krastano...@gmail.com]

I was checking the code again recently and now I have a few new
questions about the adaptive sampling (this is part of a gsoc project,
however the questions are to anybody interested):

- currently there is duplication of code in the sampling method for
line plots and parametric line plots. Is it going to get better
abstracted, or is this a low priority issue for now?
- currently it overrides get_segments and get_points. Won't it be
cleaner if it overrides only get_points? Why not? (I think it will be
cleaner because the base LineSeries class already implements
get_segments based on get_points, and I would really hate to have the
logic reimplemented multiple time, as it is now)
- What is the plan about implementing variable recursion depth?
- What is the plan about disabling the adaptive sampling?
- What happens currently to the number_of_points option?
- Is there an issue about implementing this also for 3D line plots
(not 3D surface plots which would need triangular meshes)?
- Is there an issue about the much harder problem for 3D plots?

[Bharath M R]

On Tuesday, August 7, 2012 4:46:45 PM UTC+5:30, Stefan Krastanov wrote:

I was checking the code again recently and now I have a few new
questions about the adaptive sampling (this is part of a gsoc project,
however the questions are to anybody interested):

 - currently there is duplication of code in the sampling method for
line plots and parametric line plots. Is it going to get better
abstracted, or is this a low priority issue for now?

I haven't figured out how to abstract it better. Because they have to be

implemented differently for parametric and line plots 

 
 - currently it overrides get_segments and get_points. Won't it be
cleaner if it overrides only get_points? Why not? (I think it will be
cleaner because the base LineSeries class already implements
get_segments based on get_points, and I would really hate to have the
logic reimplemented multiple time, as it is now)

Yes. It would be better. But the problem is maintaining the order of those points i.e

if you want to insert new points between two points, then you will have to create

a new list. Adding to the same list is difficult. Also, the recursion code will be more

complex as it should be changed to something iterative rather than recursive. 

 - What is the plan about implementing variable recursion depth? 

 - What is the plan about disabling the adaptive sampling?
 - What happens currently to the number_of_points option?

Yes. I am implementing it in the new API. The number_of_points will be used when

adaptive is set to false. You can have a look at how the new API is going to be at

[1]. 

 - Is there an issue about implementing this also for 3D line plots

Actually not. It will be easy enough. I will try implementing it. 

(not 3D surface plots which would need triangular meshes)?
 - Is there an issue about the much harder problem for 3D plots?

I don't know how to do it with triangular meshes. I will have to read up,

before I try implementing it.

[1] https://github.com/sympy/sympy/pull/1468 

[krastano...@gmail.com]

Thanks for the explanation. At some point I will move all this to the
issues list.

>> - currently it overrides get_segments and get_points. Won't it be
>> cleaner if it overrides only get_points? Why not? (I think it will be
>> cleaner because the base LineSeries class already implements
>> get_segments based on get_points, and I would really hate to have the
>> logic reimplemented multiple time, as it is now)
>
>
> Yes. It would be better. But the problem is maintaining the order of those
> points i.e
> if you want to insert new points between two points, then you will have to
> create
> a new list. Adding to the same list is difficult. Also, the recursion code
> will be more
> complex as it should be changed to something iterative rather than
> recursive.

Python lists have a very quick insert() method. Is this sufficient?

>>
>> - What is the plan about implementing variable recursion depth?
>>
>> - What is the plan about disabling the adaptive sampling?
>> - What happens currently to the number_of_points option?
>
> Yes. I am implementing it in the new API. The number_of_points will be used
> when
> adaptive is set to false. You can have a look at how the new API is going to
> be at
> [1].

I have mentioned it on the PR page: the API is constrained by what is
implemented in the Series classes. It would be a bad idea to implement
it twice or to document it twice. All those options are already in
each of the Series classes. It would be unmaintainable to repeat it.

>>
>> - Is there an issue about implementing this also for 3D line plots
>
> Actually not. It will be easy enough. I will try implementing it.
>>
>> (not 3D surface plots which would need triangular meshes)?
>> - Is there an issue about the much harder problem for 3D plots?
>
> I don't know how to do it with triangular meshes. I will have to read up,
> before I try implementing it.
>
> [1] https://github.com/sympy/sympy/pull/1468
>

> --
> You received this message because you are subscribed to the Google Groups
> "sympy" group.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/sympy/-/tEkYbwpPZPoJ.
> To post to this group, send email to sy...@googlegroups.com.
> To unsubscribe from this group, send email to
> sympy+un...@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/sympy?hl=en.

[Bharath M R]

On Wednesday, August 8, 2012 3:23:49 PM UTC+5:30, Stefan Krastanov wrote:

Thanks for the explanation. At some point I will move all this to the
issues list.

>>  - currently it overrides get_segments and get_points. Won't it be
>> cleaner if it overrides only get_points? Why not? (I think it will be
>> cleaner because the base LineSeries class already implements
>> get_segments based on get_points, and I would really hate to have the
>> logic reimplemented multiple time, as it is now)
>
>
> Yes. It would be better. But the problem is maintaining the order of those
> points i.e
> if you want to insert new points between two points, then you will have to
> create
> a new list. Adding to the same list is difficult. Also, the recursion code
> will be more
> complex as it should be changed to something iterative rather than
> recursive.

Python lists have a very quick insert() method. Is this sufficient?

Yeah. it might be sufficient, but I think it will still be more complex.

I will give it a try sometime after I finish the plot api. 

I have mentioned it on the PR page: the API is constrained by what is
implemented in the Series classes. It would be a bad idea to implement
it twice or to document it twice. All those options are already in
each of the Series classes. It would be unmaintainable to repeat it.

All these options will be added to the Series class itself. The only change

is that these values are passed onto the Series class.

I think documentation is necessary, thought it is repeated. For somebody,

who is using the plot_*, it becomes necessary to have the options mentioned

in the plot API. It will be difficult to figure it out from the Series docstrings.

I am open to removing those options, if people feel it is bad practice / unnecessary.

Thanks,

Bharath M R