Tilt conversion is broken

[joelvbernier]

Just noticed that this is not working properly -- more to follow.

[psavery]

Yes, here is the function that does this. We might not be doing everything quite right in it.

Basically, if the conversion is from one convention to another (such as "XYZ Extrinsic" to "ZXZ Intrinsic"), the conversion attempts to go like this:

XYZ Extrinsic -> Matrix Invariants -> ZXZ Intrinsic

If the conversion is from one convention (such as "XYZ Extrinsic") to None, it goes like this:

XYZ Extrinsic -> Matrix Invariants

If the conversion is from None to a convention (such as "XYZ Extrinsic"), it goes like this:

Matrix Invariants -> XYZ Extrinsic

The block that converts to the Matrix invariants is basically as follows:

rme = RotMatEuler(np.zeros(3), old_axes, old_extrinsic)
tilt_array = np.array(detector_tilt)
rme.rmat = makeRotMatOfExpMap(tilt_array)
detector_tilt = list(rme.angles)

The block that converts from the Matrix invariants to the convention is basically as follows:

rme = RotMatEuler(np.zeros(3), new_axes, new_extrinsic)
rme.angles = np.array(detector_tilt)
phi, n = angleAxisOfRotMat(rme.rmat)
detector_tilt = (phi * n.flatten()).tolist()

Let us know if something is wrong with the logic or the code.

[joelvbernier]

Looks correct; but I noticed for an example what I "update" to intrinsic ZXZ, it looks like the form view it is just converting the existing exponential map parameter to degrees.

the source is

em = np.r_[-0.0004610845068592427, -2.2212338648031222, -2.22123386480326]

and the output to form vies after update is: And when I chose to convert back to "none" and update, I get the correct angles,

panel.tilt
Out[196]: array([-4.61084507e-04, -2.22123386e+00, -2.22123386e+00])

np.degrees(angles_from_rmat_zxz(panel.rmat))
Out[197]: array([1.80000000e+02, 9.00000000e+01, 2.37869558e-02])

angles_from_rmat_zxz(panel.rmat)
Out[198]: (3.1415926535897927, 1.5707963267948344, 0.00041516069743255916)

but represented as expmap numbers So something is out of phase in the update?

[psavery]

@joelvbernier Yeah, I just realized that I had accidentally swapped the order for the conversions to the matrix invariants and back. I'm about to put up a PR to fix it. Thank you for the detailed comment!

However, I did notice one thing. When I convert those example tilt angles to "Intrinsic ZXZ", sometimes the first number is "-180" instead of "+180". This makes the images appear different in the viewers. Is this expected? It is seemingly random whether it comes out as "+180" or "-180".

A similar thing happens when converting to "Extrinsic XYZ". The third axis is sometimes "+180", and sometimes it is "-180". And the images in the viewer appear different depending on which one it is.

[joelvbernier]

Hmmm... I'll have to check when it "flips". The calculation should be deterministic, up to roundoff. There is the fact that ±180° leave you in the same configuration, physically speaking.

[joelvbernier]

It looks like the update of the instrument through the current conversion might be incorrect. The behavior switching back and forth is still not quite there.

[psavery]

Hi Joel,

I might need to see an example where it doesn't work. I think it works for the example above:

em = np.r_[-0.0004610845068592427, -2.2212338648031222, -2.22123386480326]

If I start hexrd and set the Euler angle convention to None, and then read in a file with those tilt angles, the detector spinboxes look like these:

And then, if I change the Euler angle convention to "Intrinsic ZXZ", and select "Yes" for updating the current tilt angles, the detector spinboxes look like these:

Don't those look like the correct tilt angles?

I can switch the convention back to "None" to get the original numbers again.

[psavery]

I do see that the sliders are still displayed in degrees, even when the convention is None. I can work on fixing that...

[psavery]

@joelvbernier

[psavery]

There also seems to be a problem where the tilt angles are not always displayed in the tree view... but they are displayed in the form view.

[cryos]

Can you provide the receipt to reproduce this @joelvbernier? As I remember what we looked at you had the two panels, and changed the angle convention, then moving the panel made it disappear, changing back to none didn't fix it, and the slider panel still had degrees displayed.

[psavery]

It sounds like a lot of those might be caused by the slider widget not recognizing the None euler angle convention. I am on it.

[joelvbernier]

Once this Paul request is merged, I will try it again and see where we are at. Once I can verify that it is behaving properly, we can close the issue. I didn’t have a question about what the dialog box asking to update the angles is actually doing?

Sent from my iPhone

On Oct 17, 2019, at 11:28, Patrick Avery notifications@github.com wrote:

It sounds like a lot of those might be caused by the slider widget not recognizing the None euler angle convention. I am on it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[psavery]

So, if you choose "No" for updating the angles, it just keeps the tilt values and re-labels them with the new convention, rather than converting the values to the new convention.

I thought it would be useful for things like, if you load a config file that uses a different convention than the current one, you can switch the convention after loading without actually modifying the values.

I can get rid of the question, though, and always convert them if that's preferred.

[joelvbernier]

Probably can toss it — config standard going forward will be matrix invariants.

Sent from my iPhone

On Oct 17, 2019, at 12:57, Patrick Avery notifications@github.com wrote:

So, if you choose "No" for updating the angles, it just keeps the tilt values and re-labels them with the new convention, rather than converting the values to the new convention.

I thought it would be useful for things like, if you load a config file that uses a different convention than the current one, you can switch the convention after loading without actually modifying the values.

I can get rid of the question, though, and always convert them if that's preferred.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[psavery]

It is removed in the PR mentioned above.

[cryos]

Is this resolved now? If not, what is left to do?

[joelvbernier]

Not fixe yet, I have to debug where things are going wrong internally.

-J

On Nov 4, 2019, at 7:14 AM, Marcus D. Hanwell notifications@github.com wrote:

Is this resolved now? If not, what is left to do?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cryos/hexrdgui/issues/177?email_source=notifications&email_token=AAIZYUT4G34YPE6FOMYQB2LQSA36VA5CNFSM4IYHX5Y2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC7S4VY#issuecomment-549400151, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIZYUWNMDV55JI4N4RYQHTQSA36VANCNFSM4IYHX5YQ.

[joelvbernier]

Ok @psavery, I was taking a look at convert_tilt_convention in utils.py and here is what I have gleaned:

1. I am not sure I understand exactly how the iconfig dictionary is used internally. The convert_tilt_convention routine seems to modify the values in this dict, which I assume is tied to the various interactors in the GUI (?). The issue seems that the instrument object (is there just one in the namespace?) is not getting updated properly from this dict when used in, say, the polar view. It looks like the polar viewer, cartesian viewer, and calibration (as it stands now) all instantiate instruments on the fly; however the update routines do not take into account the tilt conversion (currently this is an attribute at the instrument level, not at the individual detector level). This is one potential source of the discrepancy.

2. Currently in hexrd3, there are complementary methods for an HEDMInstrument to yield and update itself from a concatenated list of parameters, params, as follows

   • params[:7] as the instrument-specific parameters
     • energy [keV]
     • beam azimuth [degrees]
     • beam polar angle [degrees]
     • chi [degrees]
     • tvec[0] [mm]
     • tvec[1] [mm]
     • tvec[2] [mm]
   • params[7:] are concatenated for each detector, in the order in which they are stored in the detectors dict
     • tilt[0] [dimensionless if invariants, else degrees]
     • tilt[1] [dimensionless if invariants, else degrees]
     • tilt[2] [dimensionless if invariants, else degrees]
     • tvec[0] [mm]
     • tvec[1] [mm]
     • tvec[2] [mm]
     • <N distortion parameters, if applicable>. If the tilt_calibration_mapping attribute is not None, then it uses the conversion logic via RotMatEuler for yielding calibration_parameters and updating via update_from_parameter_list().

3. There are multiple choices here... perhaps we should updated the instrument using the intrinsic update method? This would require mapping the interactor values to a global vector, unless we modify the method to take a dictionary (i.e. iconfig) instead. Otherwise we just correct the specific detector updates in the routines under /calibration (although that code is exactly what is in the insutrument's update method already). Ideally there is a single, master parameter list that is mutable through the interactors, and a single instrument that gets updated from that list. If we want, we could keep the as-loaded list in case we want to "reset".

[psavery]

I think I may have found the issue.

Some clarification, first, though. We save the dict loaded from the yaml.load() of the instrument config file as our internal config, namely instrument_config or iconfig. This internal dict is where we get the GUI values, and modifying the GUI values modifies the instrument_config.

When we start a Cartesian view or a Polar view, we create a new instrument object immediately before, here for Cartesian. We pass in our internal dict instrument_config to HEDMInstrument as the instrument_config argument.

When the user changes the Euler angle convention, we modify all the tilt angles in the internal instrument_config dict, and then it gets passed to the HEDMInstrument constructor with the appropriate RME. However, I just noticed that it looks like the HEDMInstrument constructor is also modifying all the tilt angles according to the RME also. Thus, it looks like the tilt angles are being converted twice: once in the internal config in hexrdgui, and once in HEDMInstrument.

I think we can fix this issue by always converting the instrument_config tilt angles back to the None angle convention before passing it to the HEDMInstrument constructor. What do you think, @joelvbernier ?

We can also talk this through over video chat if you want. Let me know.

[joelvbernier]

The instrument should only be modifying the values via the calibration parameters property. The actual tilt values in the property are stored as invariants and do not get converted. Also, the reason the calibration prayers are being yelled at as a single list is to play nice with optimization problems. This behavior could be changed though, storing it more like a dictionary and then having a utility function unwrap it into a list

Sent from my iPhone

On Nov 5, 2019, at 08:47, Patrick Avery notifications@github.com wrote:

 I think I may have found the issue.

Some clarification, first, though. We save the dict loaded from the yaml.load() of the instrument config file as our internal config, namely instrument_config or iconfig. This internal dict is where we get the GUI values, and modifying the GUI values modifies the instrument_config.

When we start a Cartesian view or a Polar view, we create a new instrument object immediately before, here for Cartesian. We pass in our internal dict instrument_config to HEDMInstrument as the instrument_config argument.

When the user changes the Euler angle convention, we modify all the tilt angles in the internal instrument_config dict, and then it gets passed to the HEDMInstrument constructor with the appropriate RME. However, I just noticed that it looks like the HEDMInstrument constructor is also modifying all the tilt angles according to the RME also. Thus, it looks like the tilt angles are being converted twice: once in the internal config in hexrdgui, and once in HEDMInstrument.

I think we can fix this issue by always converting the instrument_config tilt angles back to the None angle convention before passing it to the HEDMInstrument constructor. What do you think, @joelvbernier ?

We can also talk this through over video chat if you want. Let me know.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[joelvbernier]

Also, note that the calibration parameters when they are Euler angles are yielded/input as degrees. I made a fix yesterday that allowed to reset the tilt conversion to None as well.

Sent from my iPhone

On Nov 5, 2019, at 08:47, Patrick Avery notifications@github.com wrote:

 I think I may have found the issue.

Some clarification, first, though. We save the dict loaded from the yaml.load() of the instrument config file as our internal config, namely instrument_config or iconfig. This internal dict is where we get the GUI values, and modifying the GUI values modifies the instrument_config.

When we start a Cartesian view or a Polar view, we create a new instrument object immediately before, here for Cartesian. We pass in our internal dict instrument_config to HEDMInstrument as the instrument_config argument.

When the user changes the Euler angle convention, we modify all the tilt angles in the internal instrument_config dict, and then it gets passed to the HEDMInstrument constructor with the appropriate RME. However, I just noticed that it looks like the HEDMInstrument constructor is also modifying all the tilt angles according to the RME also. Thus, it looks like the tilt angles are being converted twice: once in the internal config in hexrdgui, and once in HEDMInstrument.

I think we can fix this issue by always converting the instrument_config tilt angles back to the None angle convention before passing it to the HEDMInstrument constructor. What do you think, @joelvbernier ?

We can also talk this through over video chat if you want. Let me know.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[joelvbernier]

Let’s have a let’s have a chat. I will be at my computer in an hour.

Sent from my iPhone

On Nov 5, 2019, at 08:47, Patrick Avery notifications@github.com wrote:

 I think I may have found the issue.

Some clarification, first, though. We save the dict loaded from the yaml.load() of the instrument config file as our internal config, namely instrument_config or iconfig. This internal dict is where we get the GUI values, and modifying the GUI values modifies the instrument_config.

When we start a Cartesian view or a Polar view, we create a new instrument object immediately before, here for Cartesian. We pass in our internal dict instrument_config to HEDMInstrument as the instrument_config argument.

When the user changes the Euler angle convention, we modify all the tilt angles in the internal instrument_config dict, and then it gets passed to the HEDMInstrument constructor with the appropriate RME. However, I just noticed that it looks like the HEDMInstrument constructor is also modifying all the tilt angles according to the RME also. Thus, it looks like the tilt angles are being converted twice: once in the internal config in hexrdgui, and once in HEDMInstrument.

I think we can fix this issue by always converting the instrument_config tilt angles back to the None angle convention before passing it to the HEDMInstrument constructor. What do you think, @joelvbernier ?

We can also talk this through over video chat if you want. Let me know.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[joelvbernier]

See my comments on #213