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

Allow the option for timestamp to be generated for ns-train --load-config #2442

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

kobejean
Copy link
Contributor

When running ns-train --load-config $CONFIG_PATH currently the timestamp is fixed to what was in the config file, which is probably fine. But when I omit the timestamp parameter from the config file, it would be nice to have a new timestamp be set. Instead, the current implementation leaves the timestamp value as the default string value "{timestamp}" instead of a new valid timestamp.

I believe we can fix this with a single line change, by moving the call to config.set_timestamp() to after loading the config file. I don't see how this will impact anything else.

This won't affect people who want their timestamp fixed to what the config file reads because config.set_timestamp() only sets the value if it is set to the default value "{timestamp}" due to the if statement here:

def set_timestamp(self) -> None:
"""Dynamically set the experiment timestamp"""
if self.timestamp == "{timestamp}":
self.timestamp = datetime.now().strftime("%Y-%m-%d_%H%M%S")

I think this behavior is probably was what was intended originally but got lost overtime.

I have tested this locally on an ubuntu computer.

Please consider this PR

Cheers

@tancik tancik merged commit b0e7001 into nerfstudio-project:main Sep 19, 2023
4 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants