ENH: Reviser par Sonnet 5 - #11
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revue de code —
src/antsnormflows/(horscore.py, déjà traité)Revue de 45 fichiers (~8000 lignes) :
distributions/,flows/(+affine/,neural_spline/),nets/,sampling/,utils/restants,transforms.py,__init__.py.Statut : toutes les corrections listées ci-dessous ont été appliquées, à l'exception du renommage des deux classes
ConditionalDiagGaussian(choix explicite : documentation croisée seulement, pas de renommage, pour ne pas casser l'API publique et les tests). La factorisationGlowBlock2d/3detInvertible1x1Conv/1x1x1Conv(section "Autres observations") n'a pas non plus été effectuée, comme indiqué à l'époque — trop risqué à faire sans exécution possible des tests GPU. La syntaxe de tous les fichiers modifiés a été vérifiée (ast.parse), et les tests existants ont été relus pour confirmer qu'aucun ne dépend du comportement changé.Le problème le plus important :
log_det/log_probsansdevice=/dtype=Un même bug se répète dans au moins 8 endroits : un tenseur de log-déterminant ou de log-probabilité est créé avec
torch.zeros(...)/torch.ones(...)sans préciserdevice=. Par défaut ces tenseurs sont créés sur CPU. Si le modèle tourne sur GPU, l'opération qui combine ensuite ce tenseur avec un tenseur GPU (+=, ou pire.float()puis addition) lève soit une erreur explicite (RuntimeError: Expected all tensors to be on the same device), soit dans un cas précis (voir plus bas) uneAttributeError. C'est le même bug que j'ai corrigé dansdistributions/encoder.py-style ailleurs, mais il n'avait pas été traité ici.flows/base.pytotal_logabsdet = torch.zeros(batch_size)dansComposite._cascadeComposite(exportée publiquement) plante sur GPUflows/mixing.pylogabsdet = torch.zeros(batch_size)dans_Permutation._permuteLULinearPermute(exportée), plante sur GPUflows/mixing.pytorch.ones(outputs.shape[0])dans le chemin caché (using_cache=True) de_Linearflows/mixing.pyidentity = torch.eye(self.features, self.features)dans_LULinear.weight_inverseweight_inverse()distributions/encoder.pytorch.zeros(z.size()[0:2])dansDirac/Uniform(encoders VAE)Diracest leq0par défaut deNormalizingFlowVAE— plante sur GPU dès la première utilisation par défautdistributions/encoder.pytorch.randn((batch_size, num_samples, self.d), device=x.device)dansConstDiagGaussian.forwardAttributeErrorsix=None(cas documenté comme valide)Recommandation : passer
device=(etdtype=où pertinent) partout ci-dessus, en suivant le pattern déjà utilisé correctement ailleurs dans le même fichier (ex.distributions/base.py).Bug transverse :
log_d.float()suppose quelog_detest toujours un tenseurflows/reshape.py(Split,Merge,Squeeze2d,Squeeze3d) retournelog_det = 0(entier Python), pas un tenseur — alors queflows/base.pydéfinit justement un helperzero_log_det_like_z(z)pour éviter ça, mais ces classes ne l'utilisent pas.C'est sans conséquence tant que le log_det est seulement accumulé via
+=(un tenseur+= 0ne pose pas de problème), ce qui est le cas dansMultiscaleFlow(qui a une garde explicite.float() if torch.is_tensor(log_det_) else log_det_) et dansAffineCouplingBlock. Maiscore.py::_apply_flow_sequence(utilisée parNormalizingFlow/ConditionalNormalizingFlow) faitlog_det += log_d.float()sans cette garde. Si quelqu'un construit unNormalizingFlow(pasMultiscaleFlow) avecSplit,Merge,Squeeze2douSqueeze3ddans sa liste de flows,forward_and_log_det/inverse_and_log_detplante avecAttributeError: 'int' object has no attribute 'float'.Deux corrections possibles : (a) faire retourner à
Split/Merge/Squeeze2d/Squeeze3dun vrai tenseur viazero_log_det_like_z(z), ce qui est la correction la plus propre et cohérente avec le reste du code ; ou (b) ajouter la même gardetorch.is_tensor(...)dans_apply_flow_sequence. Je recommande (a).Bugs ponctuels
nets/mlp.pyNotImplementedError(...)construite mais jamais levée (raisemanquant) — unoutput_fninvalide est silencieusement ignoré au lieu de lever une erreurdistributions/base.pyGaussianPCA.forward/log_probfor _ in range(5): try: L = ... except RuntimeError: jitter *= 10) peut se terminer sans jamais réussir à calculerL, causant unUnboundLocalErrorconfus au lieu d'une erreur clairedistributions/encoder.pyConstDiagGaussian.forwardAttributeErrorsix=None, cas pourtant documenté et supporté par la signaturetransforms.pyShift.forward/inversez -= self.shift/z += self.shiftmodifient le tenseur en place, contrairement à toutes les autres flows du code (qui clonent avant de muter, ex.PeriodicWrap/PeriodicShift). Risque d'erreur autograd (RuntimeErrorde version counter) sizest utilisé ailleurs ou nécessite un gradientflows/mixing.pyexcept:nu (attrape tout, y comprisKeyboardInterrupt) dans_LULinear.inverse_no_cache— à remplacer par une exception préciseflows/affine/autoregressive.pyMaskedAffineAutoregressive.__init__self.features = featuresest assigné avantsuper().__init__(made)— fonctionne par chance ici (un entier ne déclenche pas la vérificationnn.Module), mais c'est un anti-pattern fragileutils/splines.pyconditional_compile = torch.compileappliqué automatiquement àsearch_sorted/unconstrained_rational_quadratic_splinedès que la variable d'environnementCIn'est pas"true"— comportement de production différent du comportement en CI, jamais testé, risque de recompilations ou d'échecs silencieux liés àtorch.compileen dehors de CIutils/splines.pyassert (discriminant >= 0).all()— lesassertsont supprimés en modepython -O; à remplacer par une vérification explicite si c'est une garde de sécurité numérique importanteDuplication de nom : deux classes
ConditionalDiagGaussiandistributions/base.py::ConditionalDiagGaussian(une distributionq0, prend uncontext_encoderréseau de neurones) etdistributions/target.py::ConditionalDiagGaussian(une distribution ciblep, oùcontextest directement[loc, scale]concaténés) portent le même nom mais n'ont rien en commun — ni la signature, ni la sémantique decontext. Aucune des deux n'est exportée à la racine du package (antsnormflows.distributions.ConditionalDiagGaussiann'existe pas), les tests important explicitementfrom antsnormflows.distributions.base import ConditionalDiagGaussianoufrom antsnormflows.distributions.target import ConditionalDiagGaussian. Risque réel de confusion / mauvais import silencieux. De même,distributions/base.py::Uniform(distributionq0non conditionnelle) est masquée pardistributions/encoder.py::Uniform(encodeur VAE conditionnel) au niveau du package :antsnormflows.distributions.Uniformrésout vers la version deencoder.py, pas celle debase.py.Recommandation : renommer l'une des deux paires de classes (ex.
base.ConditionalDiagGaussian→ConditionalDiagGaussianEncoder, outarget.ConditionalDiagGaussian→ConditionalDiagGaussianTarget), et exporter explicitement (ou explicitement exclure avec un commentaire) chaque nom ambigu dans__init__.py.Autres observations (mineures, pas bloquantes)
distributions/linear_interpolation.py::LinearInterpolationn'hérite pas denn.Modulebien qu'il enveloppe potentiellement deux distributions qui en sont. Sidist1/dist2sont entraînables, leurs paramètres ne seront pas suivis par.parameters()/.to(device)du module englobant.sampling/hais.py::HAIS.layersest une liste Python simple (pasnn.ModuleList), etHAISn'est pas unnn.Module— cohérent avec un usage "inférence seulement", mais à signaler si un jour on veut entraîner les paramètreslog_step_size/log_massdes couches HMC qu'il contient.__init__.py: leexcept Exception:autour de l'import decore.pyest très large ; en cas de vraie erreur danscore.py, l'utilisateur obtientNormalizingFlow = Nonepuis unTypeErrorconfus plus tard, au lieu du traceback original.distributions/prior.py::PriorDistributiona un__init__qui lèveNotImplementedErrorinconditionnellement (au lieu d'utiliserabc.ABC) ; les sous-classes évitent le problème simplement en ne l'appelant jamais viasuper().__init__(). Fonctionne, mais fragile si une des classes venait à changer.utils/masks.py::create_mid_split_binary_masketcreate_random_binary_masksemblent être du code mort (jamais appelés danssrc/).distributions/base.py::GaussianMixtureutilisetorch.log(torch.softmax(...))plutôt queF.log_softmax(...), moins stable numériquement.GlowBlock2d/GlowBlock3d(flows/affine/glow.py) etInvertible1x1Conv/Invertible1x1x1Conv(flows/mixing.py) sont du code quasiment dupliqué 2D/3D — factorisable, mais risqué à toucher sans tests GPU disponibles.Ce que je n'ai pas fait
Je n'ai pas vérifié en détail la justesse mathématique de chaque formule de densité (Sinusoidal, Smiley, TwoModes, etc. dans
distributions/prior.py) ni les fichiersnets/lipschitz.py(~600 lignes, code repris dertqichen/residual-flows) au même niveau de détail que le reste — repérage rapide de patterns à risque seulement, pas de relecture ligne à ligne complète.