fix: align GINOConfig parameter names with GINO.__init__ signature - #725
fix: align GINOConfig parameter names with GINO.__init__ signature#725harshaa765 wants to merge 1 commit into
Conversation
|
Thanks for finding and fixing these issues @harshaa765! Was there a reason for changing the default from nerf to transformer? If not I'd leave it to not create a new surprising behavior for existing users. Otherwise it looks good to me! |
Thanks @JeanKossaifi for the review! You are right, changing the default wasn't intentional. I'll push an update a bit later to change it back to nerf. Glad you caught that! |
GINOConfig (and GINO_Small3d) contained four parameter names that do not match the arguments accepted by GINO.__init__, causing a TypeError at model instantiation time: - gno_coord_embed_dim -> gno_embed_channels - gno_radius -> in_gno_radius + out_gno_radius (split) - gno_pos_embed_type -> in_gno_pos_embed_type + out_gno_pos_embed_type (split) - fno_domain_padding removed (not a parameter of GINO) The split pos_embed_type fields keep the original 'nerf' default to preserve existing behavior (GINO.__init__ itself defaults to 'transformer', but the config has historically used 'nerf'). The same fno_domain_padding stale field is also removed from GINO_Poisson2d, which inherits from GINOConfig. Verified by running scripts/train_gino_carcfd.py end-to-end: the model now instantiates and trains without error.
0167a1d to
b9df4d7
Compare
|
@JeanKossaifi reverted the default back to |
2 participants
Summary
GINOConfig(and its subclassGINO_Small3d) defined four config fields whose names do not match any parameter inGINO.__init__. This caused an immediateTypeErrorwhen runningscripts/train_gino_carcfd.py, becauseget_modelpasses all config fields as**kwargsdirectly to the model constructor.Changes in
config/models.py:__init__)gno_coord_embed_dimgno_embed_channelsgno_radiusin_gno_radius+out_gno_radius(split)gno_pos_embed_typein_gno_pos_embed_type+out_gno_pos_embed_type(split)fno_domain_paddingThe same stale
fno_domain_paddingfield is also removed fromGINO_Poisson2d, which inherits fromGINOConfig.Note:
GINO_Poisson2dalready used the correct split names (in_gno_radius,out_gno_radius,in_gno_pos_embed_type,out_gno_pos_embed_type,gno_embed_channels) as its own overrides — those are preserved unchanged.Test plan
scripts/train_gino_carcfd.py— model now instantiates and trains without error (verified epoch 0 and epoch 1 with decreasing loss)black config/models.py— no formatting changes needed