Remove not tested scorer #54
Conversation
This was a very experimental and not tested scorer. - fix Volume Scorer here a 1-x was missing.
- add readme text.
rollback readme
rollback readme
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
==========================================
- Coverage 96.35% 96.23% -0.12%
==========================================
Files 13 13
Lines 604 585 -19
==========================================
- Hits 582 563 -19
Misses 22 22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
For this PR the only missing thing is to remove the Defaultscorer from the example jupyter notebook. |
There was a problem hiding this comment.
The removal looks good just the notebook left to do. One potential issue is the renaming of the element_change module to atom_changes is an API break. I did a quick search on github and didn't find any users but it could break some scripts. Do we need to make this change or can we support both import paths for now and start an API deprecation process?
src/kartograf/filters/__init__.py
Outdated
| @@ -1,4 +1,4 @@ | |||
| from .element_change import ( | |||
There was a problem hiding this comment.
Because Kartograf claims a 1.0 we do indeed need to avoid the API break - it's being used in our tooling enough that we don't want to swap this out. We'll need to do a deprecation and aim for a change in 2.0.
|
I'll be finishing this for Ben. |
|
Ahh good point I'll wait for @IAlibay since I am unsure if I should self-approve. |
There was a problem hiding this comment.
I was briefly looking at this earlier and was wondering - why are we keeping this file at all? Have we been making folks import things from this location?
There was a problem hiding this comment.
From a github search it looks like others are using this import path as its the recommended way to get the scorers in the README.
There was a problem hiding this comment.
Ok thanks! Let's go with this for now, but let's raise an issue to review if it needs deprecating / removing for an eventual 2.0.
There was a problem hiding this comment.
Ok thanks! Let's go with this for now, but let's raise an issue to review if it needs deprecating / removing for an eventual 2.0.
This is an artifact which is left over from earlier endavours.
I think we should remove the untested DefaultKartograafScoerer, as this potentially confuses uses.