Making AdventV2 automatic #137
Conversation
tianyu-z
changed the title
tianyu-z
requested review from
melisandeteng,
vict0rsch,
51N84D,
alexrey88 and
sashavor
| @@ -4,10 +4,14 @@ | |||
| from pathlib import Path | |||
There was a problem hiding this comment.
Maybe you could move this file to the folder utils_scripts/ ? :) I think it would make more sense for it to be there.
train.py
Outdated
| @@ -52,6 +59,17 @@ def main(opts): | |||
| opts.output_path = str(get_increased_path(opts.output_path)) | |||
| pprint("Running model in", opts.output_path) | |||
|
|
|||
| # check if auto adventv2 works | |||
There was a problem hiding this comment.
I think you could just write:
if opts.train.lambdas.advent.is_auto_adventv2: {...}
instead of using the brackets, like it's done in the rest of the file, to keep the same formatting
omnigan/utils.py
Outdated
| return opts | ||
|
|
||
|
|
||
| def adventv2EntropySplit(trainer, verbose=1): |
There was a problem hiding this comment.
I don't know where I would put this function exacty, but I'm not sure if it fits well in a util file, specially since it's a big function that depends on another function in losses.py?
image_entropy_split.py
Outdated
| @@ -75,7 +86,7 @@ def merge_JsonFiles(filename): | |||
| with open(f1, "r") as infile: | |||
| result.extend(json.load(infile)) | |||
|
|
|||
| with open("easy_split_with_orignal_sim.json", "w") as output_file: | |||
| with open(args.save_path + "easy_split_with_orignal_sim.json", "w") as output_file: | |||
There was a problem hiding this comment.
use
from pathlib import Path
...
with (Path(args.save_path) / "easy_split_with_orignal_sim.json").open("w") as output_file:
...Because then you don't need to rely on args.save_path ending with /
omnigan/utils.py
Outdated
|
|
||
|
|
||
| def tupleList2DictList(tuples, keys=["x", "m"]): | ||
| DictList = [] |
There was a problem hiding this comment.
being peaky here but convention is to use _ instead of camel case: aaa_bbb instead of aaaBbb. + in python variables should not start with a capital letter, this is reserved for classes (though it's still syntactically correct, it will run)
omnigan/utils.py
Outdated
| with open(f1, "r") as infile: | ||
| result.extend(json.load(infile)) | ||
|
|
||
| with open(save_path + "easy_split_with_orignal_sim.json", "w") as output_file: |
There was a problem hiding this comment.
same here: use Path() to handle paths, it's way safer
| # start training if the expected training epochs is not 0 | ||
| if opts.train.epochs != 0: | ||
| trainer.train() | ||
| if is_auto_adventv2: |
There was a problem hiding this comment.
I think there needs to be a check around the number of epochs because the default is 100000000 meaning endless phase 1...
|
Small changes requested I think it would make more sense for you to walk us through the process to criticize it constructively :) Do you have reference experiments to look at how much v2 would help? |
The experiment is here. The entropy_split term for this experiment is 0.67 which means 67% of the original sim data will be in easy_split.json and 1/3 will be in hard_split.json. I have run entropy_split>0.67 and it seems 0.67 is better than other cases (by my eyeballing). Entropy_split<0.67 is still running. I am able to give the result tomorrow. |
|
Closing for now since we won't use it |
No description provided.