Add notebook to use sectorPhi

[celinedurniak]

No description provided.

[butlerpd]

ask @caitwolf and @pbeaucage and @dehoni to look at before merging. @celinedurniak would appreciate feedback.

[dehoni]

I have tested the Jupyter notebook. It is working straight-away. The examples are well-worked out and clear, and show how a user can get access to 2D data manipulation from Sasview. I am sure this notebook will serve as inspiration and important starting point for future users. There are a few suggestions to enhance the documentation (new issues outside this PR) and a few improvements.
Suggestions:

1. To demonstrate the occasional user further use cases, it might be worth to link to a documentation of the dataloader, which offers an overview of the standardly available ROI (Rings, sectors, azimuthal averages, boxes).

Improvements:
1)Please check again that the sign convention for angles is identical to Sasview. The counterclockwise sense is correct, but Phi=0 should point to the right direction.
2)For the plot_settings in "Visualize azimuthal sector" and "Case 2: Disc" I get a MatplotLib Deprecation Warning: "python3.7/site-packages/ipykernel_launcher.py:6: MatplotlibDeprecationWarning: shading='flat' when X and Y have the same dimensions as C is deprecated since 3.3. Either specify the corners of the quadrilaterals with X and Y, or pass shading='auto', 'nearest' or 'gouraud', or set rcParams['pcolor.shading']. This will become an error two minor releases later."
3) It is worrying that for "Case 2 Disc" the integrated intensity is not identical for the 4 sectors (with normalized area for each quadrant). This is an unexpected behaviour and needs further investigation. It seems that the main cause is that qx and qy bins are not centred symmetrically around 0. This also produces the impression that the disc or later the box is offset towards positive q values. This might be a general issue. To avoid this issue, should one rebin q to have the value in the middle of a bin rather than the bin edges? Does this demand to interpolate data (with fractional bins)? Needs discussion...

[krzywon]

As of this today, the sas.sascalc.dataloader has moved to its own package outside the main sasview repository called sasdata.

You will need to update your references and clone the package:

• sas.sascalc.dataloader.data_info -> sasdata.dataloader.data_info
• sas.sascalc.dataloader.manipulations -> sasdata.data_util.manipulations

[caitwolf]

I have reviewed the notebook and appreciate the provided examples; I learned a lot from them while I was going through the review! I have the following comments and suggestions:

Note: Items 1-3 echo those described by @dehoni above and I've only included them here for additional comments.

1. I agree, the sign convention should start with phi=0 pointing along the positive x-axis; the "Sign convention" figure should be updated to show this but I believe the sectors in the rest of the notebook are applying the angle phi correctly with phi=0 to the right.
2. I ran into the same MatplotLib Deprecation Warning.
3. I agree this is due to the qx and qy binning. However, I did notice that the circle/disk was being centered around qx=0.001 and qy=0.001 with the currently implemented indexing rather than qx=0, qy=0 due to the even number of qx and qy points centered around 0 but not at zero, i.e., values of -0.001 and 0.001 are in the qx and qy points. The section of code generating the disk could be slightly modified to something like:

for i, x in enumerate(qx_data_1d):
    for j, y in enumerate(qy_data_1d): 
        if np.sqrt(x**2 + y**2) <= 0.04:
            data_circle[i, j] = 1

This would result in an equal number of points in each quadrant and a constant integrated intensity.

I also note that this change only partially fixes the visual offset in the disk on the plot. I think this is due to matplotlib assuming the qx and qy points are the bin edges rather than centers as @dehoni mentions above. Perhaps there is a way to modify this setting so that the four bins surrounding the origin appear to be in their respective quadrants. I do think the underlying sectors, however, are being implemented properly based on the integrated intensities.

This also translates to the Case 3: circle centered at 0, 0 with zeros at the sectors' edges. The zeros appear to be placed when qy = 0.001 or qx = 0.001 and so they are slightly offset from the axes. The integrated intensity was constant across the quadrants since this offset was also balancing with the circle center at 0.001, 0.001 rather than 0, 0 due to the indexing described above. A similar fix described above for the the disk could be implemented here to account for the origin offset and also zeros could be placed when qx is 0.001 or -0.001 or when qy is 0.001 or -0.001. Again, this only partially fixes the visual offset on the quadrant plot but does ensure the circle is centered at (0, 0) and that the sector edges run in between the zeros along the axes.

Further discussion on this topic would be good; I made the assumption that the qx and qy values are to be interpreted as the bin/pixel centers and so if this assumption is incorrect, I will need to modify my suggestions above.

4. In the section "Visualise azimuthal sectors" I noticed that the dashed sector lines cut through the entire circle, and so if a phi_min=0 and phi_max=pi/2 were set, the dashed line would show through the phi angles 0 to pi/2 as well as pi to 3pi/2. I'm less familiar if this tool is used this way to only section off a part of the image, but if so, my suggestion would be to use the origin as one endpoint and the max radius as the other endpoint for each dash line so that if the user wants to change the phi range it will only plot the utilized section. For example:

lines_sectors = np.zeros((sect.nbins, 2, 2))

for i in range(sect.nbins):
    lines_sectors[i, 0, 1] = np.cos((sect.phi_max - sect.phi_min) / sect.nbins * i) * np.sqrt(np.max(qx_data_1d)**2+np.max(qy_data_1d**2))
    lines_sectors[i, 1, 1] = np.sin((sect.phi_max - sect.phi_min) / sect.nbins * i) * np.sqrt(np.max(qx_data_1d)**2+np.max(qy_data_1d**2))

Then when you plot the dashed sector lines, you can reference the start and stop points with:

# plot sectors' limits
for i in range(sect.nbins):
    ax.plot(lines_sectors[i,0,:], lines_sectors[i,1,:], 'c--')

[celinedurniak]

Thanks @caitwolf and @dehoni for your comments.

1. Looking at case 6 helps checking the origin of phi. The smallest values of phi corresponds to sector 3.
   Phi=0 is therefore along the x-axis starting from negative values.

But I also checked the code:

In sasdata/data_util/manipulations.py
In the doctoring of _Sector:
“Phi is defined between 0 and 2*pi in anti-clockwise
starting from the x- axis on the left-hand side”

In the code of _Sector:
phi_value = math.atan2(qy, qx) + np.pi

2. I passed shading=‘auto’ to pcolormesh to avoid the warning

3. and 4. Thanks @caitwolf for the suggestions. I modified the code accordingly.

[pbeaucage]

I finally found time to review this; I'm very sorry that it took so long.

Overall I think this is a nice demonstration of the features and use of sectorPhi.

It's not out of place for the notebooks directory, but it is a little confusing as a worked-example because it both contains some nice semi-instructional content and then the last section which feels a lot like just unit tests in long form. Right now there is a lot of code, a lot of which is unfortunately not super approachable, and not a lot of explanation.
It would be a lot more useful for novices if it had more narrative text and comments so that the user is actually guided through: what are we doing, why, how, where are the guardrails, where are the known pitfalls.

In the 'unit test' section, I'd avoid the assert statements if possible.
For experienced users they are confusing because I use them exclusively in unit tests and in establishing guardrails deep in code. (assert gravity_dir=='down', 'laws of earth broken, not sure what to do'). For inexperienced users it's a part of Python syntax they might not have seen. On the whole explaining what the data should look like and showing what it does look like is a lot more relatable.

One specific thing I'd like to call out is that the first thing a user sees in the notebook is a massive wall of matplotlib graphics commands to make the sign convention figure. This is a nice demonstration of how to do drawing in matplotlib, I guess, but it's hardly "welcome to an example on a specific science thing".... You can embed images into notebooks -- maybe just take the output of this cell, embed it where the cell currently is for the sign convention (which does need to be introduced this early), and then throw the generation code at the end of the nb for users who really want to see it?

[pbeaucage]

see earlier comment

[celinedurniak]

@pbeaucage thanks for the review.

1. I added a short introduction, removed the asserts and tried to describe more when printing results .
2. I removed the code generating the figure showing the convention for the sign of Phi and inserted an image.

I hope these modifications make the notebook clearer and easier to use.

[pbeaucage]

This looks great, thank you!

[caitwolf]

Thanks @celinedurniak, the code is looking good! You are correct about phi=0 pointing along the negative qx axis. Requested changes before merge:

• Can you make a fix to the code I suggested in item 4 above and that was implemented in your notebook? I had assumed phi=0 along the positive qx axis and so it just needs to be modified slightly with an additional np.pi in the cos and sin functions so sectors defined with a phi range less than 0 to 2pi will show in the correct quadrant.

[caitwolf]

Outside of this pull request but in regards to the phi=0 convention, @butlerpd and I did some investigating and this shift was implemented to account for the SasView gui convention of phi=0 along the negative qx axis and a range of -pi to pi. However, this is confusing if one switches between scripting in the gui, and so I'm working on a separate issue that describes this in greater detail so the conventions can be made consistent moving forward. I'll link the new issue here when it's created for future reference.

[celinedurniak]

Thanks @caitwolf
I corrected the definition of the angles for sector_lines.

[caitwolf]

Thanks @celinedurniak!