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

probability function is wierd #233

Merged
merged 2 commits into from
Aug 18, 2023
Merged

probability function is wierd #233

merged 2 commits into from
Aug 18, 2023

Conversation

nforsg
Copy link
Contributor

@nforsg nforsg commented Aug 18, 2023

mixed_ppo_policy.py

Det är något med probability som inte går ihop. Den returnerar det uppenbart booleanska uttrycket self.action(o) == a, men type-hintingen säger att det ska vara en int som returneras. Dokumentationskommentaren säger att man ska returnera sannolikheten att erhålla heltalet a, och en sannolikhet är ett flyttal mellan 0-1. Dessutom är self.action en NDArray (har kollat PPO:s dokumentation) och inget heltal som a är, vilket också borde ge error vid körning (?).

Jag vet inte riktigt hur den här funktionen ska se ut, för den ska returnera en sannolikhet right?

@Limmen
Copy link
Owner

Limmen commented Aug 18, 2023

I Python så är "True" och "False" keywords för 1 och 0. Men den funktionen ser ändå lite konstig ut. Jag antar att superklassen vill att probability ska vara en float inte int. Så du kan ändra typehinten till float och sen ändra till return float(self.action(o=o) == a).

Du kan även ändra kommentaren under def action() från "Multi-threshold..." till "Mixed PPO policy".

@Limmen
Copy link
Owner

Limmen commented Aug 18, 2023

Angående NDArray, denna type hintingen är nog fel: https://github.com/Limmen/csle/blob/master/simulation-system/libs/csle-common/src/csle_common/dao/training/ppo_policy.py#L54

Notera [0] på slutet:

a = self.model.predict(np.array(o), deterministic=False)[0]

Dvs model.predict returnerar en array men funktionen returnerar en int. Om mypy inte fattar att det är en int kan du kanske skriva return int(a)

@Limmen
Copy link
Owner

Limmen commented Aug 18, 2023

Kollade PPO doc och du har rätt att returntypen från predict är Tuple[np.ndarray, Optional[Tuple[np.ndarray, ...]]]

Men den optional returneras inte i vårt fall så vi får bara en NdArray tillbaka och då borde [0] ge första elementet i array.

@Limmen
Copy link
Owner

Limmen commented Aug 18, 2023

Jag testkörde det precis och det stämmer det jag sa ovan. Men type hintingen för PPO verkar inte vara helt korrekt, det jag får tillbaka från predict() är (int, None), så när vi kör [0] får vi en int istället för en NdArray.

@nforsg
Copy link
Contributor Author

nforsg commented Aug 18, 2023

n() från "Multi-threshold..." t

Oh juste, jag glömde att ändra type-hinten när jag gjorde den där ändringen ja!

@nforsg
Copy link
Contributor Author

nforsg commented Aug 18, 2023

Jag testkörde det precis och det stämmer det jag sa ovan. Men type hintingen för PPO verkar inte vara helt korrekt, det jag får tillbaka från predict() är (int, None), så när vi kör [0] får vi en int istället för en NdArray.

Ja precis om det är så att vi får en int tillbaka så är deras dokumentation fel helt enkelt, då dom säger att första elementet i tuplen ska vara en NDArray och inget annat. Men bra då har jag koll

@nforsg
Copy link
Contributor Author

nforsg commented Aug 18, 2023

Något som jag skulle kunna ha gjort fel är att type self.model som PPO. Nu när jag kör help(PPO.load) i IDLE så ser jag att det finns en return-typing som är ~SelfBaseAlgorithm method of abc.ABCMeta instance, vilket jag har svårt att hitta dokumentation kring. Däremot säger dom också att det just PPO.load returnerar en ny model med "loaded parameters", vilket känns som en PPO. Så även om det blir rätt i action kan just type-hintingen av self.model vara fel, ska se om jag kan rätta till det.

@Limmen
Copy link
Owner

Limmen commented Aug 18, 2023

Något som jag skulle kunna ha gjort fel är att type self.model som PPO. Nu när jag kör help(PPO.load) i IDLE så ser jag att det finns en return-typing som är ~SelfBaseAlgorithm method of abc.ABCMeta instance, vilket jag har svårt att hitta dokumentation kring. Däremot säger dom också att det just PPO.load returnerar en ny model med "loaded parameters", vilket känns som en PPO. Så även om det blir rätt i action kan just type-hintingen av self.model vara fel, ska se om jag kan rätta till det.

Jag tror type hintingen är korrekt. PPO är en subklass av SelfBaseAlgorithm

@Limmen Limmen merged commit c562e4c into master Aug 18, 2023
0 of 38 checks passed
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.

2 participants