Small simulator improvements for v0.3 #114
Conversation
|
Will deal with #23 here too, adding "binary" and (potentially) "sinc" as options |
|
This is good to review now, coveralls fails (and coverage "falls") because I've managed to drag the number of code lines down (and thus reduced our percentage coverage). |
There was a problem hiding this comment.
Basically looks good except I'm not totally sold on the name "excitation_function" - I've suggested "shape_factor_model"? But feel free to merge yourself @pc494 either with or without that change at your discretion.
| @@ -106,9 +113,16 @@ def calculate_ed_data( | |||
| rotation : tuple | |||
| Euler angles, in degrees, in the rzxz convention. Default is (0,0,0) | |||
| which aligns 'z' with the electron beam | |||
| excitation_function : function or str | |||
There was a problem hiding this comment.
I think this should be called the "shape_factor" or maybe "shape_factor_model" given that we use "shape_factor" in the code already? I.e. I would teach that the diffracted wave amplitude is the product of the structure factor (evaluated at the reciprocal lattice vector, g) and the shape factor (evaluated at the excitation error, s_g). E.g. also done in this book. https://www.springer.com/gp/book/9783642297601
Do you see where I'm coming from, or think I'm being silly?
There was a problem hiding this comment.
on consideration I would agree that shape_factor_model is better, will change and merge.
There was a problem hiding this comment.
Any preference for whether I define the shape_factor_model as an option in the shape_factor list (line c. 165), or whether it should have it's own generator of some form and then get imported into the diffraction generator?
There was a problem hiding this comment.
shape_factor_models should be defined by users in their notebooks. I hadn't anticipated adding others to the code base in the short term. If you do want to add them, I would opt for functions defined in this file, with an example in the docstrings. Making linear and binary work as they do is only really acceptable because they are so short (and have no parameters).
There was a problem hiding this comment.
I think we probably want some "standard" shape_factor_model functions to be available to users - if only because it's a way that bugs could be introduced and by standardizing them we can iron that out over time. I.e. I thought the plan was to at least add a basic sinc function that many users may want?
We don't want everyone within the community, or even within one research group, ending up with slightly different functions written for the same purpose, I think. Obviously it's different if it's a very niche implementation.
But I agree with @pc494 that these should be defined as functions in this file with examples and docstrings if they're going to go in, and I'd be ambivalent about instead creating a demo notebook that defines a few functions for shape factors so that it's "documented" and "standard" but also not in the code.
Do we have opinions on which of those options would be the happier compromise?
There was a problem hiding this comment.
I've been swung, we should provide models for users as part of the code. I have some thoughts about the exact syntax but I think I'll raise a separate issue to clear that up, as it'll also illustrate why "binary" and "linear" are not just simpler but also different from many of the other functions we might consider
name: Small simulator improvements for v0.3
about: Some housekeeping from recent PR's and also addressing #110
Release Notes
What does this PR do? Please describe and/or link to an open issue.
Makes a number of internal changes to improve code a readability
Changes the function signatures of the methods of DiffractionGenerator as well as that classes'
__init__, moves some args over to being keyword arguments