Conversation
pzhanggit
left a comment
pzhanggit
left a comment
There was a problem hiding this comment.
See my comments. The PR will require more rounds of reviews. One major comment is to move the gno as part of the embedding and debedding as we discussed earlier this week.
| if len(variables) == 3: | ||
| if len(variables) >= 3: | ||
| ret_dict["cond_fields"] = variables[2] | ||
| if len(variables) >= 4: |
There was a problem hiding this comment.
Does geometry always show up together with cond_fields?
| indices_x = slice(6, 54) | ||
| indices_y = slice(1, 49) | ||
| indices_z = slice(1, 49) |
There was a problem hiding this comment.
What are those hard-coded numbers?
| else: | ||
| dictcase[datacasedir]["stats"] = self.compute_and_save_stats(f, json_path) | ||
|
|
||
| nx = [50, 192, 50] |
There was a problem hiding this comment.
could we use self.cubsizes?
| tx = np.linspace(0, nx[0], res[0], dtype=np.float32) | ||
| ty = np.linspace(0, nx[1], res[1], dtype=np.float32) | ||
| tz = np.linspace(0, nx[2], res[2], dtype=np.float32) | ||
| self.geometry = np.stack(np.meshgrid(tx, ty, tz, indexing="ij"), axis=-1) |
There was a problem hiding this comment.
What does the variable self.geometry represent? It seems it will be combined with indices to point out the solid objects location?
| from .models.avit import build_avit | ||
| from .models.svit import build_svit | ||
| from .models.vit import build_vit | ||
| from .models.gno import build_gno |
There was a problem hiding this comment.
Would build_gno support a stand-alonegino model? If not, I suggest not to put it together with other models. Maybe treat it more like a utils function? OR rename it to something likeadd_gno`?
| cond_input = None | ||
|
|
||
| if "geometry" in data: | ||
| geometry = data["geometry"] |
There was a problem hiding this comment.
Not on this PR, but now I wonder if we should simply keep the data dictionary objects unwrapped...
| def __init__(self, inner_model, params=None): | ||
| super().__init__() | ||
|
|
||
| num_channels = params.gno["n_channels"] |
There was a problem hiding this comment.
An example config file for gno is missing?
There was a problem hiding this comment.
Btw, I suggest we set some default values for those variables
| # reduction=params.gno.reduction | ||
| ) | ||
|
|
||
| self.model = inner_model |
| bmin = [0, 0, 0] | ||
| bmax = [1, 1, 1] | ||
| self.latent_geom = self.generate_geometry(bmin, bmax, self.res) |
There was a problem hiding this comment.
Either make them configurable or hard-coded in generate_geometry
| out = torch.zeros(B, C, D, H, W, device=x.device) | ||
| out_model = rearrange(out, 'b c d h w -> b (h w d) c') | ||
| out = rearrange(out, 'b c d h w -> b (h w d) c') |
There was a problem hiding this comment.
Confusing lines - why don't you define it as b (h w d) c?
|
Closing in favor of #37. |
2 participants
No description provided.