[Feat] Add DACT and NeuOpt Improvement Models #184
Conversation
| @@ -291,3 +291,57 @@ def __setstate__(self, state): | |||
| self.__dict__.update(state) | |||
| self.rng = torch.manual_seed(0) | |||
| self.rng.set_state(state["rng"]) | |||
|
|
|||
|
|
|||
| class ImprovementEnvBase(RL4COEnvBase, metaclass=abc.ABCMeta): | |||
There was a problem hiding this comment.
[Minor] I'm good with the current for the moment, however this covers more 1)routing 2)euclidean space only
There was a problem hiding this comment.
We can keep this for now though 👍
rl4co/models/zoo/n2s/encoder.py
Outdated
| @@ -194,7 +196,7 @@ def __init__( | |||
| feedforward_hidden=feedforward_hidden, | |||
| ) | |||
|
|
|||
| assert self.env_name in ["pdp_ruin_repair"], NotImplementedError() | |||
| assert self.env_name in ["pdp_ruin_repair", "tsp_kopt"], NotImplementedError() | |||
There was a problem hiding this comment.
Does it necessarily have to be of this kind, i.e., not generalizable for now to new envs? (at least the base logic)
There was a problem hiding this comment.
Good point! Removed already!
| @@ -212,7 +212,7 @@ def forward( | |||
| td.set("action", N2S_action) | |||
|
|
|||
| if return_embeds: | |||
| outdict["embeds"] = h_wave | |||
| outdict["embeds"] = h_wave.detach() | |||
There was a problem hiding this comment.
Why do we detach() here? They are not used anywhere else?
There was a problem hiding this comment.
This is because the critic takes the detached values by design for my methdos
rl4co/models/zoo/neuopt/decoder.py
Outdated
| self.init_parameters() | ||
|
|
||
| def init_parameters(self) -> None: | ||
| for param in self.parameters(): |
|
|
||
| self.init_parameters() | ||
|
|
||
| def init_parameters(self) -> None: |
There was a problem hiding this comment.
This one is kept since we have nn.Parameters that need initialization
rl4co/models/zoo/neuopt/policy.py
Outdated
| """ | ||
|
|
||
| # Encoder: get encoder output and initial embeddings from initial state | ||
| NFE, _ = self.encoder(td) |
There was a problem hiding this comment.
[Minor] if data is being passed around, i.e. embeddings in this case, I suggest changing naming with lowercase letters (i.e. NFE -> nfe), since uppercase is usually reserved for classes
There was a problem hiding this comment.
Yeah, changed already
tests/test_training.py
Outdated
| trainer.test(model) | ||
|
|
||
|
|
||
| def test_NeuOpt(): |
There was a problem hiding this comment.
[Minor] same minor comment as above:
- variables, functions ...: lowecase
- classes: uppercase (at least first letter(
There was a problem hiding this comment.
Yeah, changed already
|
Awesome! No more comments, I think ready to merge @cbhua at your will |
Description
Types of changes
What types of changes does your code introduce? Remove all that do not apply:
Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!