Skip to content

Conversation

@yoshikisd
Copy link
Collaborator

Makes the surface normal vector a parameter in Bragg2DPtycho.from_dataset to make it easier for folks to define what the surface normal is.

Copy link
Collaborator

@allevitan allevitan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice, seems like this is a super helpful feature!

Right now, if I'm reading the code right, it's written so that if there is an 'orientation' defined in dataset.sample_info, then that will override the provided surface_normal argument, and the same if a 'scattering_mode' argument is set.

I think it would make the most sense if surface_normal > scattering_mode > dataset.sample_info['orientation'], i.e. an explicitly defined surface_normal overrides everything, a scattering_mode overrides the info in dataset.sample_info, and dataset.sample_info serves as a fallback. Does that make sense?

@yoshikisd
Copy link
Collaborator Author

Makes sense to me, and I think that sounds like a good idea. The latest push has this hierarchy implemented.

Currently, it's set so that surface_normal > scattering_mode > dataset.sample_info['orientation'] > np.array([0, 0, 1]) (default transmission mode). There is a RuntimeError implemented to prevent surface_normal from being anything other than a 3-element numpy vector. However, issues with scattering_mode or dataset.sample_info['orientation'] may silently cause a default transmission geometry to be used for the surface_normal. Maybe it might be a good idea to introduce guards for that as well, perhaps in a different PR?

… an array with np.asarray, change the check for length-3 to cover all cases, and add a check to explicitly fail if the scattering_mode argument is improperly set, instead of silently falling back to default
@allevitan
Copy link
Collaborator

I just added a few suggested tweaks to the branch, let me know if they look good:

  • I made the scattering_vector argument work with anything that can cast to a numpy array with np.asarray
  • I broadened the check for length 3
  • I stopped it from silently falling back to default if an invalid scattering_mode is given
  • I slightly broadened the allowed strings for 'r' and 't' mode.

If it looks good, let me know and I can clear my comments and merge it.

@yoshikisd
Copy link
Collaborator Author

Thanks for making these changes; they all look good to me. Would you do the honors of merging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants