-
Notifications
You must be signed in to change notification settings - Fork 21
Check valid x and y array lenghts and xtypes in insert_scattering_quantity
#209
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
==========================================
+ Coverage 96.54% 96.62% +0.08%
==========================================
Files 8 8
Lines 347 356 +9
==========================================
+ Hits 335 344 +9
Misses 12 12
|
|
||
@xtype.setter | ||
def xtype(self, _): | ||
raise AttributeError(_setter_wmsg("xtype")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge Like Would you also like to prevent the user from directly modifying xtype
?
@@ -276,7 +289,7 @@ def get_array_index(self, value, xtype=None): | |||
""" | |||
|
|||
if xtype is None: | |||
xtype = self.input_xtype | |||
xtype = self._xtype |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do not want the user from directly modifying xtype
, like _all_arrays
, _xtype
is used internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you changed input_xtype to xtype! I do think people can directly modify xtype though (@sbillinge please confirm). For example I might want to plot my diffraction object on tth first, and then change its xtype to q so I can visualize it in qspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yucongalicechen that is not how it works. the attribute was called input_xtype
because we wanted to keep a record of the xtype input by the user. So I don't want to change its name. When xtype is used for plotting any xtype can be given, for example diffobj.on_xtype('q')
would satisfy @yucongalicechen 's UC even if the data were input on a tth
grid.
Since we only use it internally, we could change the attribute name to _input_xtype
to make it private. I think it should only be set by insert_data
and not re-settable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge - done. input_type
is used internally and it st only gettable.
tests/test_diffraction_objects.py
Outdated
|
||
def test_xtype_getter(): | ||
do = DiffractionObject(xtype="tth") | ||
assert do.xtype == "tth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test getter for xtype
tests/test_diffraction_objects.py
Outdated
assert do.xtype == "tth" | ||
|
||
|
||
def test_xtype_setter(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test setter for xtype
- not allowed
@yucongalicechen could you please give me a quick review? Since you've written the issue, I want to ensure this is what was imagined. |
assert do.input_xtype == "tth" | ||
|
||
|
||
def test_input_xtype_setter(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not allowed to reset _input_data
ready for review - @sbillinge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this disappeared of my notifications but I never submitted the review
def all_arrays(self, _): | ||
raise AttributeError(_setter_wmsg("all_arrays")) | ||
|
||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we decide that we don't want as setter for input_xtype? Sorry, I am getting a bit confused here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
@input_xtype.setter
def input_xtype(self, _):
raise AttributeError(_setter_wmsg("input_xtype"))
This code basically, gives AttributeError
when the user attempts to modify input_xtype
manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not looking far enough down. Thanks for the explanation
@@ -347,12 +369,17 @@ def insert_scattering_quantity( | |||
if wavelength is not None: | |||
self.wavelength = wavelength | |||
|
|||
# Check xtype is valid. An empty string is the default value. | |||
if xtype != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we delete this line? I think empty string is not in XQUANTITIES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From our current init implementation, if xtype
is not provided for DiffractionObjects
, then xtype
becomes an empty string as shown here:
class DiffractionObject:
def __init__(
self, name=None, wavelength=None, scat_quantity=None, metadata=None, xarray=None, yarray=None, xtype=None
):
...
self.metadata = metadata
if xtype is None:
xtype = ""
Hence, we need to have if xtype != ""
to ensure that we do not provide the warning msg when no value is provided for xtype
, as you mentioned, ""
is not in XQUANTITIES
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I guess this is left over from our/my desire to allow people to instantiate an empty DO and then fill it later. Let's leave it like that, but we may be accumulating annoying technical debt that we wouldn't have if we went away from that choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbillinge ready for review - didn't modify any code but replied to your in-line comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see comments. It all looks good, modulo a nasty feeling that we may have made an imperfect choice....
def all_arrays(self, _): | ||
raise AttributeError(_setter_wmsg("all_arrays")) | ||
|
||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def all_arrays(self, _): | ||
raise AttributeError(_setter_wmsg("all_arrays")) | ||
|
||
@property |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not looking far enough down. Thanks for the explanation
@@ -347,12 +369,17 @@ def insert_scattering_quantity( | |||
if wavelength is not None: | |||
self.wavelength = wavelength | |||
|
|||
# Check xtype is valid. An empty string is the default value. | |||
if xtype != "": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I guess this is left over from our/my desire to allow people to instantiate an empty DO and then fill it later. Let's leave it like that, but we may be accumulating annoying technical debt that we wouldn't have if we went away from that choice.
Closes #200