Move model code into new repo and some bug fixes #21
Conversation
slider/curve.py
Outdated
| @@ -131,10 +125,28 @@ def __call__(self, t): | |||
| class Passthrough(Curve): | |||
| kinds = 'P' | |||
|
|
|||
| def __init__(self, points, req_length): | |||
| def __new__(cls, points, req_length): | |||
There was a problem hiding this comment.
The changes in this module are actually unrelated and should probably have been pulled out. ping @blimmo
|
This is actually producing interesting results now! I am not sure how I am going to support training for users when the upload files, but I can figure that out later. @blimmo when you get a chance, can you review the new curve module changes in here? |
slider/curve.py
Outdated
| for kind in cls.kinds: | ||
| cls._kind_dispatch[kind] = cls | ||
|
|
||
|
|
||
| class Bezier(Curve): | ||
| kinds = 'L' | ||
| kinds = () |
There was a problem hiding this comment.
Shouldn't this be an empty string to be consistent with the other kinds?
There was a problem hiding this comment.
I normally use the unit tuple for empty sequences. Strings are used elsewhere because they have the nice property that they are iterables of characters. To be more clear we could make the other kinds tuples as well. What do you think?
There was a problem hiding this comment.
I'd rather use the empty string here so we can continue to use multicharacter strings for multiple types but if we were going to move the curve type choice logic up into from_kind_and_points anyway isn't this going to be removed?
There was a problem hiding this comment.
yeah, I guess we should just have a map internally there, no need for this registration magic
slider/curve.py
Outdated
| @@ -128,13 +123,42 @@ def __call__(self, t): | |||
| return self._curves[bi]((t - pre_t) / (post_t - pre_t)) | |||
|
|
|||
|
|
|||
| class Passthrough(Curve): | |||
| kinds = 'P' | |||
| class LinearMetaCurve(MetaCurve): | |||
There was a problem hiding this comment.
Is this possible? Don't all multi curve sliders use 'B'?
There was a problem hiding this comment.
Looking at the source of osu! it appears that the Linear curve splits into subcurves between each point: https://github.com/ppy/osu/blob/7fbbe74b65e7e399072c198604e9db09fb729626/osu.Game/Rulesets/Objects/SliderCurve.cs#L30
The particular slider that I saw this was and old favorite: https://osu.ppy.sh/s/19789
For the new lstm stuff I ran through all of my maps and collected stats about all of the locations and the old interpretation here would cause positions on the scale of 1e23.
slider/curve.py
Outdated
| kinds = 'P' | ||
|
|
||
| def __new__(cls, points, req_length): | ||
| if len(points) != 3: |
There was a problem hiding this comment.
It seems a bit weird to be returning a bezier instance when you called the perfect constructor. Wouldn't it be better to put this logic in the from_kind_and_points method? Then the user could forcibly get a perfect curve if they called the perfect constructor and would get a valueerror of the points were bad.
There was a problem hiding this comment.
That makes sense, I will lift this up there.
slider/curve.py
Outdated
| 0.5 * (p1.y + p2.y) - t * (p2.x - p1.x), | ||
| ) | ||
| if np.isclose([a_squared, b_squared, c_squared], 0).any(): | ||
| raise ValueError() |
There was a problem hiding this comment.
If we do allow the user to see this error by calling the perfect constructor then this should probably have a little more description.
There was a problem hiding this comment.
I agree. These checks guard against collinear points, I will put that in the error message with the coordinates.
slider/curve.py
Outdated
| def _init(self, points, req_length, center): | ||
| self.points = points | ||
| self.req_length = req_length | ||
| self._center = center |
There was a problem hiding this comment.
I just realized that this isn't being flipped in HR. I think we should just add a hard_rock attribute here to compute this so the code in Slider can just call that.
There was a problem hiding this comment.
note to self: this could cause training noise on hr maps
|
I do think that the model code should be in a separate library. I'm using slider in a program that I use cx_freeze on so I try to minimize my dependencies and it feels weird to have all this model code (even if most of it isn't actually there). Mainly I just think it makes more sense tbh although I might be being biased because I don't understand it :P. Eh, ignore all that, I vote take it out but feel free to ignore me. |
|
I suspected that people would feel that way. I will break out just the bug fixes from this PR and move all of the model code to combine so that it is still easily accessible. |
llllllllll
changed the title
|
I think I addressed your feedback, does everything look good? This pr mostly changed to just rip the models out and cleanup some bugs found while really poring through all of my replay data. |
EDIT: this pr now moves the model into a new repo instead of adding the new model