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

Fix misc bugs and issues #11

Merged
merged 9 commits into from
Feb 2, 2023
Merged

Fix misc bugs and issues #11

merged 9 commits into from
Feb 2, 2023

Conversation

FabienRoger
Copy link
Collaborator

No description provided.

@Butanium
Copy link

Butanium commented Feb 2, 2023

Won't that break imports using elk. ? Like

# generation_main.py
from elk.utils_generation.generation import create_records, create_hiddenstates

@FabienRoger
Copy link
Collaborator Author

FabienRoger commented Feb 2, 2023

To cd in elk? No, because elk is pip installed module. Using elk. works anywhere.

(That's the benefit of using a module, otherwise you can't even run python scripts from within subfolders of your projects because it doesn't know about its parent folder, if i'm not mistaken.)

Copy link
Member

@norabelrose norabelrose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have one little code style nit about line 60 of train.py but that could be changed in a later PR or something

@@ -54,6 +57,7 @@ def train(args):

# save models
# TODO: use better filename for the pkls, so they don't get overwritten
Path(args.trained_models_path).mkdir(parents=True, exist_ok=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: args.trained_models_path is already a Path so we don't have to make it one here

@norabelrose norabelrose merged commit 4b020ee into main Feb 2, 2023
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

3 participants