Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suche nach Refinements auf der Basis von Bins #254

Merged
merged 16 commits into from
Oct 5, 2020

Conversation

LukasEberle
Copy link

Hallo,
ich hab hier mal wieder den Ersten Entwurf des Codes und würde mich über Feedback freuen.

Copy link
Collaborator

@michael-rapp michael-rapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ein Paar Kleinigkeiten sind mir aufgefallen...

python/boomer/common/cpp/rule_refinement.cpp Outdated Show resolved Hide resolved
python/boomer/common/cpp/rule_refinement.cpp Outdated Show resolved Hide resolved
python/boomer/common/cpp/rule_refinement.cpp Outdated Show resolved Hide resolved
python/boomer/common/cpp/rule_refinement.cpp Outdated Show resolved Hide resolved
python/boomer/common/cpp/rule_refinement.cpp Outdated Show resolved Hide resolved
@michael-rapp michael-rapp linked an issue Oct 2, 2020 that may be closed by this pull request
@michael-rapp michael-rapp changed the title Approximate Rule Refinement Implementation Suche nach Refinements auf der Basis von Bins Oct 2, 2020
@michael-rapp
Copy link
Collaborator

Für mich sieht der Code jetzt absolut in Ordnung aus. Daher könnte ich diesen Pull-Request jetzt mergen. Allerdings habe ich in der Zwischenzeit den Pull-Request #251 in den "development"-Branch gemergt. Darin sind alle Refactorings enthalten, die von meiner Seite aus noch notwendig waren um approximative Verfahren zu unterstützen. Leider waren dafür auch ein paar Änderungen notwendig, die mit dem Code in diesem Pull-Request in Konflikt stehen.

Wir haben jetzt zwei Möglichkeiten:

  • Wir könnten diesen Pull-Request nach "approximate-conditions" mergen und dann in einem separaten Pull-Request den aktuellen Stand aus "development" ebenfalls in diesen Branch mergen und dabei alle Konflikte beseitigen.

  • Wir könnten erst den aktuellen "development"-Branch nach "approximate-conditions" mergen und die Konflikte als Teil dieses Pull-Requests beseitigen.

Ich würde dir die Entscheidung überlassen, wie wir das machen.

Hier ein Überblick der Dinge die noch angepasst werden müssten, um die Konflikte auszuräumen:

  • Die Klasse IRuleRefinement heißt jetzt AbstractRuleRefinement und deren Funktion findRefinement hat jetzt keinen Rückgabewert mehr. Stattdessen wird das gefundene Refinement in einem Klassenattribut gespeichert.
  • Das Array vom Typ BinArray kann nicht mehr als Konstruktorargument an die Klasse ApproximateRuleRefinementImpl übergeben werden. Stattdessen gibt es jetzt ein Interface für ein Callback, das aufgerufen werden kann um das Array innerhalb der findRefinement-Methode zu erhalten.
  • Die Felder indexedArray und indexedArrayWrapper wurden wie schon angekündigt aus dem struct Refinement entfernt.

@LukasEberle
Copy link
Author

Sollen wir dafür nochmal ein Call machen oder wie lösen wir das am Besten?

@michael-rapp
Copy link
Collaborator

Sollen wir dafür nochmal ein Call machen oder wie lösen wir das am Besten?

Wie es dir am besten passt. Eigentlich sind die notwendigen Änderungen nicht so dramatisch. Wenn dir klar ist, was geändert werden muss, dann brauchen wir keinen Call. Falls etwas unklar ist, können wir das aber auch gerne zusammen durchgehen.

@LukasEberle
Copy link
Author

Gerade bei dem Git Teil bin ich mir unsicher. Die nötigen Änderungen sollten klar sein nur müsste ich mir für den zweiten Punkt nochmal das Interface anschauen.

@michael-rapp
Copy link
Collaborator

michael-rapp commented Oct 2, 2020

Gerade bei dem Git Teil bin ich mir unsicher.

Wenn ich dich richtig verstehe, dann willst du die Konflikte noch in diesem Pull-Request beseitigen. Wenn ja, dann kann ich das mergen übernehmen und du müsstest dann nur noch den Code anpassen.

Die nötigen Änderungen sollten klar sein nur müsste ich mir für den zweiten Punkt nochmal das Interface anschauen.

Du musst nur ein Objekt, das dieses Interface implementiert, statt binArray als Konstruktorargument entgegennehmen und dann in findRefinement die get-Methode aufrufen. Das wird in ExactRuleRefinementImpl genau so gemacht. Das einzige was sich ändert ist der Template-Typ (entspricht in etwa Generics in Java).

@LukasEberle
Copy link
Author

Ja das wird wohl das Beste sein. Lass uns das so machen

@michael-rapp
Copy link
Collaborator

Ok, die Änderungen vom "development"-Branch sind jetzt sowohl in "approximate-conditions", als auch in diesem Branch. Jetzt musst du nur noch bei dir lokal ein Git Pull machen und die Anpassungen durchführen.

@LukasEberle
Copy link
Author

Ich glaube den Callback habe ich noch nicht ganz verstanden.
Mein Verständnis ist: Wir rufen callback_->get() und bekommen einen relevanten Typen zurück. callback_ ist dabei natürlich das Klassenattribut, welches im Konstruktor zugewiesen wird.

Der Type scheint aber auf IndexedFloat32Array* festgelegt zu sein. (Ich bekomme den Kompilererror: cannot convert IndexedFloat32Array* to BinArray in initialization).

Aber wir brauchen schon ein BinArray, da wir nur so an die Anzahl der Bins kommen und natürlich auch an die Bins von denen wir ja dann alle 3 Werte (Anzahl der Beispiele, Min und Max) brauchen.

Wahrscheinlich habe ich aber irgendwas übersehen.

@michael-rapp
Copy link
Collaborator

Du muss den Typ des Konstruktorarguments und des Klassenattributes ändern von IRuleRefinementCallback<IndexedFloat32Array>* zu IRuleRefinementCallback<BinArray>*.

@LukasEberle
Copy link
Author

So, jetzt sollte es funktionieren. Danke für den Hinweis, da hab ich echt geschlafen.

@michael-rapp
Copy link
Collaborator

Ich habe wie immer Kommentare hinzugefügt und die Formatierung leicht angepasst. Außerdem ist mir aufgefallen, dass der Destruktor um das Callback zu zerstören noch gefehlt hat. Da das nur eine Kleinigkeit ist, habe ich das einfach schnell ergänzt.

Da ich ansonsten nichts mehr sehe, was noch getan werden müsste, merge ich diesen Pull-Request.

@michael-rapp michael-rapp merged commit 401b1f0 into approximate-conditions Oct 5, 2020
@michael-rapp michael-rapp deleted the approximate-rule-refinement branch October 5, 2020 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suche nach Refinements auf der Basis von Bins
3 participants