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

Verbosity and error handling improvements in bulk_register #50

Conversation

MTLehner
Copy link
Contributor

@MTLehner MTLehner commented Mar 31, 2024

Multiple small improvements to the verbosity and error prints in the function bulk_register

  1. A progress bar for bulk_register
    • A lightweight tqdm bar function is added
    • The progress bar is by default turned off and can be activated for long jobs, to get some ETA estimations
  2. Changes to function arguments for consistent snake case naming
  3. Catch kekulize error in MolToV3KMolBlock

@greglandrum
Copy link
Contributor

I'm not a fan of progress bars, but if we're going to add one it seems like it makes sense to use tqdm, instead of rolling our own. tqdm is a lightweight dependency, wouldn't it make sense to just use that?

lwreg/utils.py Outdated Show resolved Hide resolved
@greglandrum
Copy link
Contributor

Though doing an informative error message certainly makes sense, it feels like it would be good to find an actual solution to the conformer hashing problem. There's really no reason for us to be storing the whole string of coordinates as a hash (this was just me not thinking through the consequences completely), we could just generate the SHA256 from the string and store that instead.

It would help keep the database size down too, and that's not a terrible thing.

It would be a separate PR and would require having some code to transition existing lwreg installs, but that's not a big deal.

We can quickly talk about it later this week with @brje01 and I'll do that PR.

@MTLehner
Copy link
Contributor Author

MTLehner commented Apr 1, 2024

I'm not a fan of progress bars, but if we're going to add one it seems like it makes sense to use tqdm, instead of rolling our own. tqdm is a lightweight dependency, wouldn't it make sense to just use that?

I did it in my own fork with tqdm and it worked great, but adds the additional dependency on tqdm. Which is pretty light, and easy to use. Discussion with @brje01 lead us to the even lighter function, taken from here: https://stackoverflow.com/questions/3160699/python-progress-bar
Which looks and works similar to tqdm, but without all the extra options.

@greglandrum
Copy link
Contributor

As we discussed, I agree that making the argument naming scheme consistent is a good idea (even though it's a bit of a disrupitive change). The tutorial and demo notebooks also need to be updated to reflect that change.

Copy link
Contributor

@greglandrum greglandrum left a comment

Choose a reason for hiding this comment

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

LGTM

@greglandrum greglandrum merged commit 61de110 into rinikerlab:main Apr 5, 2024
1 check 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.

2 participants