Implement spectrum copy(), *, *= methods, --scale command-line option
#285
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more. @@ Coverage Diff @@
## master #285 +/- ##
==========================================
+ Coverage 95.78% 95.79% +0.01%
==========================================
Files 28 28
Lines 3958 3992 +34
Branches 798 803 +5
==========================================
+ Hits 3791 3824 +33
- Misses 98 99 +1
Partials 69 69
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
API wrangling paid off, 11ab6cc was a very satisfying little commit to write 😅 |
| return Spectrum1D(self.x_data, summed_y_data, | ||
| x_tick_labels=self.x_tick_labels, | ||
| metadata=metadata) | ||
| return Spectrum1D(np.copy(self.x_data), |
There was a problem hiding this comment.
Does this also want to be copyed?
There was a problem hiding this comment.
A copy of Spectrum1DCollection would be a Spectrum1DCollection, but this method returns a Spectrum1D.
In principle we could make a copy from e.g. the first element of the collection, but then the constructor is getting called once to make the element from the y_data and again to copy it.
It feels "different enough" to justify breaking the pattern, but I don't have a strong opinion on whether it is better to do it one way or the other.
| with pytest.raises(AssertionError): | ||
| check_spectrum1d(spec, spec * 2.) | ||
|
|
||
| def test_copy(self): |
There was a problem hiding this comment.
Is this something that wants to be fixtured?
There was a problem hiding this comment.
Did you have a particular kind of fixture in mind?
I don't see an elegant way to parametrize all of these different operations, but pytest has a lot of features...
There was a problem hiding this comment.
True, the awkward, ugly way to do it would be a list of tuples of ("attr", operation) and a loop through them, but that gets pretty ugly pretty quickly and cleanliness here is key.
There was a problem hiding this comment.
Could maybe define the check as a kind of teardown operation and make lots of little tests, but that's pesky in its own way.
I could tidy this a bit by defining an assert_different function that hides the pytest.raises?
There was a problem hiding this comment.
e.g.
def test_copy(self):
def assert_different(spectrum, other) -> None:
with pytest.raises(AssertionError):
check_spectrum1dcollection(spectrum, other)
spec = get_spectrum1dcollection('gan_bands.json')
spec.metadata = {'Test': 'item', 'int': 1}
spec_copy = spec.copy()
# Copy should be same
check_spectrum1dcollection(spec, spec_copy)
# Until data is edited
spec_copy._y_data *= 2
assert_different(spec, spec_copy)
spec_copy = spec.copy()
spec_copy._x_data *= 2
assert_different(spec, spec_copy)
spec_copy = spec.copy()
spec_copy.x_tick_labels = [(1, 'different')]
assert_different(spec, spec_copy)
spec_copy = spec.copy()
spec_copy.metadata['Test'] = spec_copy.metadata['Test'].upper()
assert_different(spec, spec_copy)
In practice we will usually want __imul__, so makes sense to define __mul__ using it. This requires a fresh Spectrum to write into. In principle it would be more efficient to create one with an "empty" data array... ... but as there is lots of code making modified array copies, life is a lot simpler with a consistent copy() method used as a base for this stuff. If copying the data becomes a noticeable bottleneck we could add an `empty=False` argument that allows us to construct with e.g. `y_data=np.empty_like(self.y_data)*ureg(self.y_data_unit)`, but for now assume YAGNI! With the new Python/numpy/Pint versions we can use np.copy() directly on an array Quantity, which reduces unit-shuffling boilerplate.
Shallow copies may lead to surprises when elements are mutated; _usually_ it will contain literals (and the type-hinting says it is _supposed_ to only take literals) but a user might put something mutable in there.
|
Rebasing onto the updated tests, not expecting drama but would be better to find out here. |
Closes #219
In order to make the
--scaleoption beautiful, tweak the Python API so that any Spectrum can by multiplied by a scalar Real using*or*=. (These are implemented with "dunder" methods__mul__and__imul__respectively.)In practice we will usually use
__imul__, so makes sense to define__mul__using it, creating a fresh Spectrum to write into. In principle it would be more efficient to create one with an "empty" data array, but as there is lots of code making modified array copies, life is a lot simpler with a consistentcopy()method used as a base for this stuff. If copying the data becomes a noticeable bottleneck we could add anempty=Falseargument that allows us to construct with e.g.y_data=np.empty_like(self.y_data)*ureg(self.y_data_unit), but for now assume YAGNI!With the new Python/numpy/Pint versions we can use np.copy() directly on an array Quantity, which reduces unit-shuffling boilerplate. 😄
To-do
Spectrum.copy()Spectrum.copy()*,*=--scaleargument--scaleargument