Conversation
dn070017
left a comment
dn070017
left a comment
There was a problem hiding this comment.
Hi Ceren, here's some of my comments, you can first check the MUST tag and tries to fix some of the issue (or we can discuss if other features needs to be implemented, or we can ignore those things first), for NITS or ASK or IMO, you don't necessarily need to change anything, just some of my thought on the code.
There was a problem hiding this comment.
[MUST] the scripts related to HPC can be put into maybe additional directory like scripts or scripts/sbatch
| n_layers: int = 2, #3 | ||
| base_n_neurons: int = 32, #128 | ||
| max_n_neurons: int = 64, #2048 | ||
| rate: int = 2, | ||
| activation: str = "swish", #"linear" |
There was a problem hiding this comment.
[NITS] I think it is safe to remove the old parameters in the comments (or we can keep them just for the ease of development)
| from cavachon.environment.constants import Constants | ||
|
|
||
|
|
||
| class PeriodicTSNECallback(tf.keras.callbacks.Callback): |
There was a problem hiding this comment.
[IMO] I think we may need a callback directory to put this (to keep things modular)
| self.model.compile( | ||
| use_vanilla_kl=False, | ||
| use_both_kl=True, | ||
| optimizer=optimizer, | ||
| loss_weights=loss_weights, | ||
| ) |
There was a problem hiding this comment.
[MUST] For the first part (parent to child), I thought we should set use_vanilla_kl=True, use_both_kl=False (note that in each iteration in the for-loop, we focus on the child component)
| ) | ||
|
|
||
| # Set initial KL weights | ||
| self.model._vanilla_kl_weights[parent_name].assign(0.0) |
There was a problem hiding this comment.
[ASK] This sets the weight properly, but I'm not sure if there's any conflict with the self.model.compile configuration,
|
|
||
| callbacks_progressive.append( | ||
| ComponentWeightTransferCallback( | ||
| total_epochs=n_progressive_epochs, |
There was a problem hiding this comment.
[NITS] In the future we may have to set total_epochs to n_progressive_epochs // 2, and next phase n_progressive_epochs - n_progressive_epochs // 2 (just write it down for the record)
| # PROGRESSIVE SECTION - Phase 2 (Child Only) | ||
|
|
||
| if self.run_progressive_training.get(component_name): | ||
| parent_name = self._get_parent_component(component_name) |
There was a problem hiding this comment.
[MUST] Parent name may be more than one (one child can have more than one parent), so we may have to tranverse the list (or set), and set the weight for all parents first, and then set the child.
(See commments above)
| ) | ||
| if len(conditioned_on_z_hat) > 0: | ||
| return conditioned_on_z_hat[0] | ||
| return None |
There was a problem hiding this comment.
return List[str] # keep zero-length list if no parent
| if self.run_progressive_training.get(component_name): | ||
| parent_name = self._get_parent_component(component_name) | ||
|
|
||
| if parent_name is not None: |
There was a problem hiding this comment.
for parent_name in parent_name_list
dn070017
left a comment
dn070017
left a comment
There was a problem hiding this comment.
LGTM, just a side note that originally CAVACHON supports conditioned on z, the code implemented now only support conditioned on z_hat (we can fix this later as most of the user will probably just conditioned on z_hat)
Note that I only review the code statically, please run some tests before merging to develop branch
|
remember to use squash merge (you can select from the expand error on the right of merge pull request) |
dn070017
left a comment
dn070017
left a comment
There was a problem hiding this comment.
Please check some of my comments, and make sure the r_networks is fixed (we might also need to fix the bias)
For other comments labeled as MUST, but you don't want to fix it now, you can add a # TODO: explain what we should do next
comment so we can quickly refer to them in the future
|
|
||
| # Find modality for saving results | ||
| modality_names = component_config.get("modality_names", []) | ||
| modality = modality_names[0] if modality_names else component_name |
There was a problem hiding this comment.
[MUST] Ideally we should allow for the case that one component output multiple modalities (put a comment which contains TODO so we can come back to unfix features in the future quickly)
| for dim in parent_dims: | ||
| W_parts.append(W[offset : offset + dim, :]) | ||
| offset += dim | ||
| W_child = W[offset:, :] |
There was a problem hiding this comment.
[MUST] W_child is scaled using r_networks. Check https://github.com/dn070017/CAVACHON/blob/develop/cavachon/modules/base/hierarchical_encoder.py
So basically, the psedo-code is like
b_networks([z_parent_1, z_parent_2..., r_networks(z_child)])
So you might have to take r_networks parameters into count for z_child.
Note that originally there's also bias in the r_networks and b_networks, maybe you can disable it to make sure the approximation works. Just add use_bias=False to the dense layer defined in https://github.com/dn070017/CAVACHON/blob/develop/cavachon/modules/base/hierarchical_encoder.py#L58
2 participants
for review