feature/issue-19/implement-drawing-error-features#20
Conversation
alperkent
added
enhancement
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #20 +/- ##
===========================================
- Coverage 100.00% 97.69% -2.31%
===========================================
Files 4 6 +2
Lines 115 130 +15
===========================================
+ Hits 115 127 +12
- Misses 0 3 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@Asanto32 I decided to exclude MSE and RMSE calculations for now, I'll explain why on our Wednesday meeting and if we decide to add them nonetheless I'll just commit it here before merging this PR. |
Asanto32
left a comment
Asanto32
left a comment
There was a problem hiding this comment.
Some small comments, the main question is why use shapely do begin with and not just scipy integration like you do in the test?
| """Feature extraction module for drawing error-based metrics in spiral drawing data.""" | ||
|
|
||
| import numpy as np | ||
| from shapely import geometry, ops |
There was a problem hiding this comment.
Check their license and how it impacts using their package with our license. Not sure what BSD-3-Clause license is
There was a problem hiding this comment.
BSD-3-Clause looks pretty permissive, so I don't think there would be any issues:
BSD 3-Clause License
Copyright (c) 2007, Sean C. Gillies. 2019, Casper van der Wel. 2007-2022, Shapely Contributors.
All rights reserved.Redistribution and use in source and binary forms, with or without
modification, are permitted provided that the following conditions are met:
Redistributions of source code must retain the above copyright notice, this
list of conditions and the following disclaimer.Redistributions in binary form must reproduce the above copyright notice,
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution.Neither the name of the copyright holder nor the names of its
contributors may be used to endorse or promote products derived from
this software without specific prior written permission.THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
| lambda x: np.abs(np.sin(x) - np.sin(x + np.pi)), -np.pi / 2, 6 * np.pi / 4 | ||
| ) | ||
|
|
||
| class MockSpiral: |
There was a problem hiding this comment.
Shouldn't have a dedicated class within a unit test
There was a problem hiding this comment.
I actually couldn't find a better way of doing this test so I left it like this hoping you might suggest a better way lol
| y1 = np.sin(x) | ||
| y2 = np.sin(x + np.pi) | ||
|
|
||
| expected_area, _ = integrate.quad( |
There was a problem hiding this comment.
This begs the question: If we can use integration to find the area under the curve here why do we need the shapely method?
There was a problem hiding this comment.
We can use integration for this test which uses only sine waves, but the spiral data is a lot trickier. My initial goal was to convert Cartesian coordinates to polar and just use np.trapz but there are some issues with conversion to polar in the beginning segment of the data and resampling the reference spiral at the same angles with the drawn spiral, so I've decided not to do this. I will explain more during our meeting tomorrow.
| self.data = data | ||
| self.metadata = metadata | ||
|
|
||
| monkeypatch.setattr(models, "Spiral", MockSpiral) |
There was a problem hiding this comment.
No part of this mocking is needed?
There was a problem hiding this comment.
I want the feature extraction functions to input the spiral object rather than only the needed columns of the dataframe, because I think it would make it easier to just call all of them with the same inputs in the orchestrator. I did not want to just import the whole dataframe instead of the spiral object because I thought maybe some features that we will extract later might need to use the metadata info like task name or handedness, or we could decide to prioritize getting a feature like task duration first and instead if calculating it separately for each other feature that uses it, we could just add it to the spiral object.
Should I just input the valid spiral fixture and change the data to this sine wave data? Because instantiating a spiral object without proper columns and metadata just throws errors.
|
|
||
| def test_calculate_area_under_curve(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """Test that the area under the curve is calculated correctly.""" | ||
| x = np.linspace(-np.pi / 2, 6 * np.pi / 4, 100) |
There was a problem hiding this comment.
Why such a complicated x-axis data points? If you just do 0-2pi I'm pretty sure this is a nice easy calculation for AUC of the difference (and an exact value)
There was a problem hiding this comment.
I basically wanted to test that the shapely method is able to handle intersecting lines. And this area is equal to 8 but I just wanted to confirm with the integrate function.
Asanto32
left a comment
Asanto32
left a comment
There was a problem hiding this comment.
Some small comments, the main question is why use shapely do begin with and not just scipy integration like you do in the test?
…at] for area under curve calculations and Hausdorff metrics, and refactor drawing error tests to use pytest fixture instead of monkeypatch
2 participants
Resolves #19.