Rename id to uuid in DiffractionObject
#271
Conversation
| @id.setter | ||
| def id(self, _): | ||
| raise AttributeError(_setter_wmsg("id")) | ||
| @uuid.setter |
There was a problem hiding this comment.
while User still has access to uuid, we make it non-settable still.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
=======================================
Coverage 98.68% 98.68%
=======================================
Files 8 8
Lines 379 379
=======================================
Hits 374 374
Misses 5 5
|
| @@ -50,7 +50,7 @@ class DiffractionObject: | |||
| The array containing the quantity of q, tth, d values. | |||
| input_xtype : str | |||
| The type of the independent variable in `xarray`. Must be one of {*XQUANTITIES} | |||
| id : uuid | |||
| _uuid : uuid | |||
There was a problem hiding this comment.
if we are making this attribute public and giving it a getter, I wonder if we should just call it uuid and remove the underscore? What do you think? It may also be less confusing if the attribute name and how it is accessed is the szme?
There was a problem hiding this comment.
Yes, I agree. It turns out we shall write a separate docstring for @property methods as shown below (as done in the latest commit):
@property
def uuid(self):
"""The unique identifier for the DiffractionObject instance.
Returns
-------
uuid
The unique identifier of the DiffractionObject instance.
"""
return self._uuid
This way, User does not have to read private properties starting with _.
API doc is also clearly marked with @property:
| input_xtype : str | ||
| The type of the independent variable in `xarray`. Must be one of {*XQUANTITIES} | ||
| id : uuid | ||
| The unique identifier for the diffraction object. |
There was a problem hiding this comment.
These are marked with @property - docstrings are provided separately (please see below)
|
@sbillinge thanks for your review - ready for review, with my reply above: #271 (comment) |
| Returns | ||
| ------- | ||
| ndarray | ||
| The 2D array containing the `xarray` values in q, tth, d, and `yarray`. |
There was a problem hiding this comment.
for lower cognitive load I would change to The shape(len(data), 4)2D array with columns containing theyarray(intensity) and thexarray values in q, tth, and d . so they appear in the docstring in the same order as in the array columns
There was a problem hiding this comment.
Yes, great! superb. The example and this docstring makes it super clear.
|
|
||
| Examples | ||
| -------- | ||
| >>> my_do.all_arrays[:, 0] # yarray |
There was a problem hiding this comment.
add "To access specific arrays individually, use these slices:"?
There was a problem hiding this comment.
There's a stray y-array at the end of the line :(
There was a problem hiding this comment.
Sorry - fixed now:
The shape (len(data), 4) 2D array with columns containing the `yarray` (intensity)
and the `xarray` values in q, tth, and d.
| @@ -319,7 +341,8 @@ def get_array_index(self, value, xtype=None): | |||
|
|
|||
| Returns | |||
| ------- | |||
| the index of the value in the array | |||
| list | |||
There was a problem hiding this comment.
let's check. Logically, this should return an int not a list. Let's check and maybe update the function/tesnts, or understand why it returns a list and change the name.
There was a problem hiding this comment.
Yup, I created a separate issue for this since it's beyond the scope of this PR. I will look into this.
|
@sbillinge ready for review - addressed your comments. thanks for the review |
|
@sbillinge ready for review - fixed the extra yarray |
Closes #269