-
Notifications
You must be signed in to change notification settings - Fork 0
Real world version #3
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
Conversation
| from sklearn.metrics import accuracy_score, roc_auc_score | ||
| from model import Net # Local model definition | ||
| import socket | ||
| import platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
socket et platform non utilisées dans le code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
il faut aussi séparer les bibliothèques par groupe (bibliothèques standard (argparse), bibliothèques tierces (pandas, torch, flwr) et importations locales (from model import Net))
| """ | ||
| Get model parameters as a list of NumPy arrays. | ||
| """ | ||
| return [val.cpu().numpy() for val in self.mo]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.model je pense? et j'ai pas compris pour les parenthèses à la fin
| The MEDfl package requires *python 3.9* or more to be run. If you don't have it installed on your machine, check out the following link [Python ](https://www.python.org/downloads/). | ||
| It also requires MySQL database | ||
| The MEDfl package requires *python 3.9* or more to be run. If you don't have it installed on your machine, check out the following link [Python](https://www.python.org/downloads/). | ||
| It also requires MySQL database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python >=3.9
two spaces between "installed" and "on"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you also give the link to install MySQL?
| It also requires MySQL database. | ||
|
|
||
| ### Package installation | ||
| For now, you can install the ``MEDfl``package as: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now?
| The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software. | ||
|
|
||
| THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pourquoi as tu enlevé la license ici et la mention du consortium MEDOMICS?
| Configuration for Differential Privacy (DP) settings. | ||
| Attributes: | ||
| noise_multiplier (float): Noise multiplier for DP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est quoi? epsilon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
est-ce que le parametre delta est inclu?
| dp_config: DPConfig = None, | ||
| ): | ||
| """ | ||
| Initialize client by loading data, creating model, optimizer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initializes*
| auc = sum(metrics.get("eval_auc", 0.0) * num_examples | ||
| for num_examples, metrics in results) / total_examples | ||
|
|
||
| return {"eval_loss": loss, "eval_accuracy": accuracy, "eval_auc": auc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Les fonctions aggregate_fit_metrics et aggregate_eval_metrics devraient être regroupées en une seule fonction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faudrait-il ajouter d'autres mesures comme std? L'aggrégation pondérée ici peut être biaisée par un centre plus populeux que les autres
| }, | ||
| { | ||
| "cell_type": "code", | ||
| "execution_count": 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
est-ce que ce fichier doit être dans le git?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attention au nom du folder (banchmarking, bachmarking, ...)
| ] | ||
| }, | ||
| { | ||
| "cell_type": "code", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commentaire en lien avec les folders de données: Il y a des données de MIMIC-Eicu dans le repo, ces données ne devraient pas être disponible ici puisque théoriquement, ce ne sont pas des données publiques.
| sphinx-rtd-dark-mode==1.2.4 | ||
| plotly==5.19.0 | ||
| optuna==3.5.0 | ||
| mysql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mysql n'était pas un package obligatoire ? Ou c'est seulement MySQL database? (selon README)
| setup( | ||
| name="MEDfl", | ||
| version="0.1.37", | ||
| version="2.0.4.dev1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.dev1?
| for num_examples, metrics in results) / total_examples | ||
| accuracy = sum(metrics.get("train_accuracy", 0.0) * num_examples | ||
| for num_examples, metrics in results) / total_examples | ||
| auc = sum(metrics.get("train_auc", 0.0) * num_examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu pourrais faire appel à une fonction générique pour appliquer la même formule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
et prévoir aussi une condition pour vérifier que total_examples est différent de 0 ? (cas rare mais possible)
| self.min_fit_clients = min_fit_clients | ||
| self.min_evaluate_clients = min_evaluate_clients | ||
| self.min_available_clients = min_available_clients | ||
| self.initial_parameters = initial_parameters or [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peut être mettre None au lieu de [] pour différencier les modèles non instanciés vs les modèles sans paramètres?
| max_grad_norm: float = 1.0, | ||
| batch_size: int = 32, | ||
| secure_rng: bool = False, | ||
| ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je sais que tu as décrit les attributs de la classe plus haut c'est pourquoi tu ne décris pas les arguments de la méthode ici, mais dans tout le reste du code tu décris les arguments et pas les attributs, peut être faire de même ici.
| # ---------- Load Data ---------- | ||
| df = pd.read_csv(data_path) | ||
| X = df.iloc[:, :-1].values | ||
| y = df.iloc[:, -1].values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plutôt ajouter deux arguments de "predictor columns" et "target column". Il arrive que certains datasets aient des colonnes d'identifiants, dates, etc qui ne sont pas utiles à la prédiction.
| self.X_tensor = torch.tensor(X, dtype=torch.float32) | ||
| self.y_tensor = torch.tensor(y, dtype=torch.float32) | ||
|
|
||
| batch_size = dp_config.batch_size if dp_config else 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ce paper parle du fait que la batch size n'affecte pas la performance du modèle, et qu'en fait, on devrait choisir le maximum que notre hardware est capable de supporter puis faire une optimisation d'hps dessus. i.e: pas besoin d'optimiser le batch size.
| # ---------- Model and Optimizer ---------- | ||
| input_dim = X.shape[1] | ||
| self.model = Net(input_dim) | ||
| self.criterion = nn.BCEWithLogitsLoss() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clairement, tu fais uniquement de la classification binaire, peut être le préciser dans l'entête de ta classe.
| # Output layer: 32 → 1 | ||
| self.fc3 = nn.Linear(32, 1) | ||
| # Dropout with 30% probability | ||
| self.dropout = nn.Dropout(0.3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De mon expérience, c'est vraiment énorme 30% de dropout
| @@ -0,0 +1,134 @@ | |||
| import flwr as fl | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ajouter un entête
| import flwr as fl | ||
| from typing import Callable, Optional, Dict, Any, Tuple, List |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entête
| self.fit_metrics_aggregation_fn = fit_metrics_aggregation_fn or aggregate_fit_metrics | ||
| self.evaluate_metrics_aggregation_fn = evaluate_metrics_aggregation_fn or aggregate_eval_metrics | ||
|
|
||
| self.strategy_object: Optional[fl.server.strategy.Strategy] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le typing n'est pas nécessaire à ce niveau
| StrategyClass = getattr(fl.server.strategy, self.name) | ||
|
|
||
| # Prepare parameters for strategy | ||
| params: Dict[str, Any] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En python, ce genre de typing n'est pas nécessaire
No description provided.