Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split Settings #50

Open
apriljunge opened this issue Mar 19, 2024 · 1 comment
Open

Split Settings #50

apriljunge opened this issue Mar 19, 2024 · 1 comment

Comments

@apriljunge
Copy link
Contributor

As for now Settings can be created by file and arguments in parallel.
In my opinion this should be split in two functions. In the current implementation it is not very logic what happens when a file and custom arguments are specified on the same time (custom arguments are currently ignored).

I think there are two solutions:

  1. Create a second overloaded Settings(). One for the file, One with parameters. Same like Path()
  2. Create a second function Settings_from_file()

I prefer the second option, as it is more clear what it does. But then Path and Train should be adapted likewise.

@kaat0
Copy link
Collaborator

kaat0 commented Mar 20, 2024

As for now Settings can be created by file and arguments in parallel. In my opinion this should be split in two functions. In the current implementation it is not very logic what happens when a file and custom arguments are specified on the same time (custom arguments are currently ignored).

I agree!

I think there are two solutions:

  1. Create a second overloaded Settings(). One for the file, One with parameters. Same like Path()
  2. Create a second function Settings_from_file()

I prefer the second option, as it is more clear what it does. But then Path and Train should be adapted likewise.

I do see your point about the second option, but:
I would prefer the overloaded solution for Settings() and Train() for the simplicity of having only to remember one function name and let the compiler do the work. A nice solution could be a forced Keyword-Argument for the file loading function to make it clear when used, e.g. Settings(file="my_mega_setting.yaml").

@kaat0 kaat0 added this to the TrainRuns.jl 2.0 milestone Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants