search

johannbrehmer / manifold-flow

Manifold-learning flows (ℳ-flows)
https://arxiv.org/abs/2003.13913
MIT License
230 stars 27 forks source link

Public update horvat #9

Closed chrvt closed 4 years ago

chrvt commented 4 years ago

1) experiments.utils.names does not exist in my version 2) parser argument "-i" twice and inconsistent with the other arguments using "--" 3) name change of file trian and test such that train.py can read them

johannbrehmer commented 4 years ago

Hi,

thanks a lot for the PR and the fixes!

May I suggest a few things before accepting the PR?

  1. The first one is totally on me. The generate_data.py was at some point moved to experiments/datasets/dataset_preparation, but somehow on the public branch the original file still kept existing in a slightly older version. I have now deleted it. Would it be possible for you to instead modify https://github.com/johannbrehmer/manifold-flow/blob/public/experiments/datasets/dataset_preparation/generate_data.py instead? Sorry about only realizing all of this now and thus making the PR non-mergable!

  2. The choice of using single dashes for one-letter CLI options was deliberate (see e.g. https://superuser.com/questions/372203/whats-the-difference-between-one-dash-and-two-dashes-for-command-prompt-paramet). I tend to agree that it is a bit inconsistent, but I'd like to keep this choice for now.

  3. I like your proposed simplification of the training filenames (especially since I never ended up comparing spheres of different dimensionalities). However, I believe the right solution would be

    filename = "{}/experiments/data/samples/{}/{}{}.npy".format(args.dir, args.dataset, label, run_label)

    just as for the other datasets (your proposal was missing the label part, which will e.g. insert the "x_train" into the filename). E.g. we could just delete the if clause.

Thanks again and sorry for all the issues. Let me know if you want to update the PR or if you would prefer me to implement these things.

Cheers, Johann

chrvt commented 4 years ago

Hi Johann,

thanks for your answers.

Sorry for the mistake in point 3. I think your proposal is the easiest one.

Regarding point 1. it seems there is no issue anymore. Thanks.

Especially thanks for explaining point 2. I should have looked this up by myself. I just thought, since there was the same parse argument twice, it was a copy/paste issue.

Thanks again and I hope my updated PR will be mergeable now since I reverted my point 1.

Best, Christian

johannbrehmer commented 4 years ago

Looks good to me, thanks again for fixing!