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

modified create splits and data processing script, and created splits… #1

Open
wants to merge 74 commits into
base: multi-property
Choose a base branch
from

Conversation

kimele2003
Copy link

… for different property sets

kevingreenman and others added 30 commits February 4, 2022 21:59
@kevingreenman
Copy link
Member

Hey, great job on the UVVisML PR! I have a few suggestions:

  • remove Miniconda3-latest-Linux-x86_64.sh file and add to .gitignore
  • remove uvvisml/Miniconda3-py37_4.8.3-Linux-x86_64.sh file and add to .gitignore
  • in the .gitignore, should the last line be "models.tar.gz.1" or "models.tar.gz"?
  • the documentation.md looks great! One suggestion for making it a bit more readable: I recommend using the `` quotes for any file or directory names in the paragraphs. It looks like you used those for file names already, but I think also using them for directories is a good idea. Eventually I think we'll want to combine this file with README.md, but it's okay in the separate file for now.
  • In each subdirectory of bash_scripts/, you have a symbolic link to either predict2.py or chemprop_sigopt_engaging.py. But I'm not sure these links are pointing to the correct places (they all point one directory up, but I the scripts they point to are not one directory up)
  • Could you move predict2.py and predict2.ipynb into the uvvisml/ directory so they'd be in the same place as uvvisml/predict.py? You'll have to change the code in these a bit to account for the new location, so run a few tests to make sure that it works like it did before from the new location

After you make these changes, I think we can merge it into a new branch on the public repo

@kevingreenman kevingreenman changed the base branch from main to multi-property May 17, 2022 16:07
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.

2 participants