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

Update database.py #257

Merged
merged 3 commits into from
Apr 2, 2024
Merged

Conversation

aakhiltayal
Copy link
Contributor

Include additional search method for pulling data from materials project

Include additional search method for pulling data from materials project
@aakhiltayal
Copy link
Contributor Author

@matthewcarbone
Please check the updated code to include additional search methods

Copy link
Contributor

@matthewcarbone matthewcarbone left a comment

Choose a reason for hiding this comment

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

Tests are failing because of permissions not because I think the code necessarily has a problem.

Can you make sure this passes some of the checks by running the tests locally? There are a ton of them so feel free to just run the tests for a few minutes. There are clearly bugs here that will probably break the tests so that's just a nice sanity check.

Thanks!

lightshow/database.py Show resolved Hide resolved
lightshow/database.py Outdated Show resolved Hide resolved
Code modified with context manager
Copy link
Contributor Author

@aakhiltayal aakhiltayal left a comment

Choose a reason for hiding this comment

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

context manager included

Copy link
Contributor

@matthewcarbone matthewcarbone left a comment

Choose a reason for hiding this comment

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

It would also be great to put an example of how this new method can be used in the docstring (which definitely needs some tlc and that's my bad).

method = _get_method(kwargs.get("method"), mpr=mpr)
try:
kwargs.pop("method")
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be except KeyError not else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

solved

Copy link
Contributor

Choose a reason for hiding this comment

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

@aakhiltayal it needs except KeyError not except. A bare except statement is dangerous because it catches everything!

Updated doc string
@aakhiltayal
Copy link
Contributor Author

It would also be great to put an example of how this new method can be used in the docstring (which definitely needs some tlc and that's my bad).

@aakhiltayal
Copy link
Contributor Author

It would also be great to put an example of how this new method can be used in the docstring (which definitely needs some tlc and that's my bad).

I agree please feel free to modify the code as you fit good.

@aakhiltayal aakhiltayal reopened this Mar 25, 2024
@matthewcarbone
Copy link
Contributor

@aakhiltayal this definitely needs an example somewhere. From your updated docstring I'm still not exactly sure how you plan on using this! Can you post a code snippet on how you do it here? I can then merge this and make some quick corrections myself after.

@matthewcarbone matthewcarbone merged commit ac025d7 into AI-multimodal:master Apr 2, 2024
4 of 13 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.

2 participants