Hi all, Running this test script http://paste.yt-project.org/show/6379/ will result in this image https://slack-files.com/T042F73QM-F0X0YJE3V-4a579d9431 Apparently, the sequence of buff_size is switched so that set_buff_size((8,4)) gives a 4 by 8 image. I have traced down the codes and found the following lines are the cause of the problem. https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/utilities/lib/pixelization_routines.pyx?at=yt&fileviewer=file-view-default#pixelization_routines.pyx-61 https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/utilities/lib/pixelization_routines.pyx?at=yt&fileviewer=file-view-default#pixelization_routines.pyx-216 If rows=nx and cols=ny, the two functions (pixelize_cartesian and pixelize_off_axis_cartesian) should take rows first and then cols. The same API has been around in _MPL.c since 2010 and thus changing it will be backward incompatible. However, to be consistent with other parts of the code (e.g. here https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/visualization/fixed_resolution.py?at=yt&fileviewer=file-view-default#fixed_resolution.py-374, here https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/visualization/fixed_resolution.py?at=yt&fileviewer=file-view-default#fixed_resolution.py-249, here https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/data_objects/data_containers.py?at=yt&fileviewer=file-view-default#data_containers.py-1388, and here https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/analysis_modules/sunyaev_zeldovich/projection.py?at=yt&fileviewer=file-view-default#projection.py-184), in which buff_size = (nx, ny) is assumed and used, I propose to change the behavior of the above two functions. What do you think? Anyone has other suggestions? Thanks! Yi-Hao
Hi Yi-Hao,
The actual usage of _MPL.c shouldn't be user-facing. So I think as
long as the things that are public APIs (such as the data returned
from a FixedResolutionBuffer object) remain the same, I am a strong +1
on this.
-Matt
On Fri, Apr 1, 2016 at 10:19 AM, Yi-Hao Chen
Hi all,
Running this test script http://paste.yt-project.org/show/6379/ will result in this image https://slack-files.com/T042F73QM-F0X0YJE3V-4a579d9431
Apparently, the sequence of buff_size is switched so that
set_buff_size((8,4))
gives a 4 by 8 image.
I have traced down the codes and found the following lines are the cause of the problem. https://bitbucket.org/yt_analysis/yt/src/d18f33211199f71e2fac4d927307b015d513a328/yt/utilities/lib/pixelization_routines.pyx?at=yt&fileviewer=file-view-default#pixelization_routines.pyx-61
If rows=nx and cols=ny, the two functions (pixelize_cartesian and pixelize_off_axis_cartesian) should take rows first and then cols.
The same API has been around in _MPL.c since 2010 and thus changing it will be backward incompatible.
However, to be consistent with other parts of the code (e.g. here, here, here, and here), in which buff_size = (nx, ny) is assumed and used, I propose to change the behavior of the above two functions.
What do you think? Anyone has other suggestions?
Thanks! Yi-Hao
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
On Fri, Apr 1, 2016 at 11:11 AM, Matthew Turk
Hi Yi-Hao,
The actual usage of _MPL.c shouldn't be user-facing. So I think as long as the things that are public APIs (such as the data returned from a FixedResolutionBuffer object) remain the same, I am a strong +1 on this.
The trouble is, if we want to make the user interface match what people expect, i.e. buff_size is (nx, ny), where nx it the number of pixels along x and ny is the number of pixels along y, we'll need to change the behavior here.
-Matt
On Fri, Apr 1, 2016 at 10:19 AM, Yi-Hao Chen
wrote: Hi all,
Running this test script http://paste.yt-project.org/show/6379/ will result in this image https://slack-files.com/T042F73QM-F0X0YJE3V-4a579d9431
Apparently, the sequence of buff_size is switched so that
set_buff_size((8,4))
gives a 4 by 8 image.
I have traced down the codes and found the following lines are the cause of the problem.
If rows=nx and cols=ny, the two functions (pixelize_cartesian and pixelize_off_axis_cartesian) should take rows first and then cols.
The same API has been around in _MPL.c since 2010 and thus changing it
will
be backward incompatible.
However, to be consistent with other parts of the code (e.g. here, here, here, and here), in which buff_size = (nx, ny) is assumed and used, I propose to change the behavior of the above two functions.
I'm +0.5 on this, but am nervous that it will change the results of user scripts. I'd like the API to match naive expectations though...
What do you think? Anyone has other suggestions?
The other alternative is to document the current (annoyingly backwards) behavior.
Thanks! Yi-Hao
Thank you for your detective work on this, Yi-Hao!
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
On Fri, Apr 1, 2016 at 11:21 AM, Nathan Goldbaum
On Fri, Apr 1, 2016 at 11:11 AM, Matthew Turk
wrote: Hi Yi-Hao,
The actual usage of _MPL.c shouldn't be user-facing. So I think as long as the things that are public APIs (such as the data returned from a FixedResolutionBuffer object) remain the same, I am a strong +1 on this.
The trouble is, if we want to make the user interface match what people expect, i.e. buff_size is (nx, ny), where nx it the number of pixels along x and ny is the number of pixels along y, we'll need to change the behavior here.
Hmmm .. but we transpose in most places, like yt/geometry/coordinates/cartesian_coordinate_handler.py .
-Matt
On Fri, Apr 1, 2016 at 10:19 AM, Yi-Hao Chen
wrote: Hi all,
Running this test script http://paste.yt-project.org/show/6379/ will result in this image https://slack-files.com/T042F73QM-F0X0YJE3V-4a579d9431
Apparently, the sequence of buff_size is switched so that
set_buff_size((8,4))
gives a 4 by 8 image.
I have traced down the codes and found the following lines are the cause of the problem.
If rows=nx and cols=ny, the two functions (pixelize_cartesian and pixelize_off_axis_cartesian) should take rows first and then cols.
The same API has been around in _MPL.c since 2010 and thus changing it will be backward incompatible.
However, to be consistent with other parts of the code (e.g. here, here, here, and here), in which buff_size = (nx, ny) is assumed and used, I propose to change the behavior of the above two functions.
I'm +0.5 on this, but am nervous that it will change the results of user scripts. I'd like the API to match naive expectations though...
What do you think? Anyone has other suggestions?
The other alternative is to document the current (annoyingly backwards) behavior.
Thanks! Yi-Hao
Thank you for your detective work on this, Yi-Hao!
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
On Fri, Apr 1, 2016 at 11:23 AM, Matthew Turk
On Fri, Apr 1, 2016 at 11:21 AM, Nathan Goldbaum
wrote: On Fri, Apr 1, 2016 at 11:11 AM, Matthew Turk
wrote:
Hi Yi-Hao,
The actual usage of _MPL.c shouldn't be user-facing. So I think as long as the things that are public APIs (such as the data returned from a FixedResolutionBuffer object) remain the same, I am a strong +1 on this.
The trouble is, if we want to make the user interface match what people expect, i.e. buff_size is (nx, ny), where nx it the number of pixels along x and ny is the number of pixels along y, we'll need to change the behavior here.
Hmmm .. but we transpose in most places, like yt/geometry/coordinates/cartesian_coordinate_handler.py .
I think the transpose of the image array is to fit the behavior of imshow(), which needs an array with the shape (ny, nx) to display nx pixels along x axis and ny pixels along y axis. I assumed generally people expect buff_size is (nx, ny). But if they really use non-square buff_size, they probably should have figured it out by trial and error and used (ny, nx) instead. The proposed fix will change this behavior.
-Matt
On Fri, Apr 1, 2016 at 10:19 AM, Yi-Hao Chen
wrote:
Hi all,
Running this test script http://paste.yt-project.org/show/6379/ will result in this image https://slack-files.com/T042F73QM-F0X0YJE3V-4a579d9431
Apparently, the sequence of buff_size is switched so that
set_buff_size((8,4))
gives a 4 by 8 image.
I have traced down the codes and found the following lines are the cause of the problem.
If rows=nx and cols=ny, the two functions (pixelize_cartesian and pixelize_off_axis_cartesian) should take rows first and then cols.
The same API has been around in _MPL.c since 2010 and thus changing it will be backward incompatible.
However, to be consistent with other parts of the code (e.g. here,
here,
here, and here), in which buff_size = (nx, ny) is assumed and used, I propose to change the behavior of the above two functions.
I'm +0.5 on this, but am nervous that it will change the results of user scripts. I'd like the API to match naive expectations though...
What do you think? Anyone has other suggestions?
The other alternative is to document the current (annoyingly backwards) behavior.
If we want to keep the current behavior and document it, many parts of the code will need to be changed as well.
Thanks! Yi-Hao
Thank you for your detective work on this, Yi-Hao!
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
Hi Yi-Hao,
OK -- I agree, this is behavior we should fix. Let's try to minimize
impact disruption if we can, but it ought to be fixed. Thanks for
digging through and finding this!
On Fri, Apr 1, 2016 at 1:44 PM, Yi-Hao Chen
On Fri, Apr 1, 2016 at 11:23 AM, Matthew Turk
wrote: On Fri, Apr 1, 2016 at 11:21 AM, Nathan Goldbaum
wrote: On Fri, Apr 1, 2016 at 11:11 AM, Matthew Turk
wrote: Hi Yi-Hao,
The actual usage of _MPL.c shouldn't be user-facing. So I think as long as the things that are public APIs (such as the data returned from a FixedResolutionBuffer object) remain the same, I am a strong +1 on this.
The trouble is, if we want to make the user interface match what people expect, i.e. buff_size is (nx, ny), where nx it the number of pixels along x and ny is the number of pixels along y, we'll need to change the behavior here.
Hmmm .. but we transpose in most places, like yt/geometry/coordinates/cartesian_coordinate_handler.py .
I think the transpose of the image array is to fit the behavior of imshow(), which needs an array with the shape (ny, nx) to display nx pixels along x axis and ny pixels along y axis.
I assumed generally people expect buff_size is (nx, ny). But if they really use non-square buff_size, they probably should have figured it out by trial and error and used (ny, nx) instead. The proposed fix will change this behavior.
-Matt
On Fri, Apr 1, 2016 at 10:19 AM, Yi-Hao Chen
wrote: Hi all,
Running this test script http://paste.yt-project.org/show/6379/ will result in this image https://slack-files.com/T042F73QM-F0X0YJE3V-4a579d9431
Apparently, the sequence of buff_size is switched so that
set_buff_size((8,4))
gives a 4 by 8 image.
I have traced down the codes and found the following lines are the cause of the problem.
If rows=nx and cols=ny, the two functions (pixelize_cartesian and pixelize_off_axis_cartesian) should take rows first and then cols.
The same API has been around in _MPL.c since 2010 and thus changing it will be backward incompatible.
However, to be consistent with other parts of the code (e.g. here, here, here, and here), in which buff_size = (nx, ny) is assumed and used, I propose to change the behavior of the above two functions.
I'm +0.5 on this, but am nervous that it will change the results of user scripts. I'd like the API to match naive expectations though...
What do you think? Anyone has other suggestions?
The other alternative is to document the current (annoyingly backwards) behavior.
If we want to keep the current behavior and document it, many parts of the code will need to be changed as well.
Thanks! Yi-Hao
Thank you for your detective work on this, Yi-Hao!
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
_______________________________________________ yt-dev mailing list yt-dev@lists.spacepope.org http://lists.spacepope.org/listinfo.cgi/yt-dev-spacepope.org
participants (3)
-
Matthew Turk
-
Nathan Goldbaum
-
Yi-Hao Chen