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

Added support for queries against Materials Project #2097

Merged
merged 18 commits into from
Oct 26, 2018

Conversation

espenfl
Copy link
Contributor

@espenfl espenfl commented Oct 23, 2018

Need to be able to pull data from the Materials Project. In this prototype, only the extraction of the structure is included. However, this can obviously be extended.

@coveralls
Copy link

coveralls commented Oct 23, 2018

Coverage Status

Coverage increased (+7.6%) to 68.572% when pulling 2a38624 on espenfl:mp_import_support into 348665e on aiidateam:develop.

@espenfl espenfl changed the title Added suppert for queries against Materials Project Added support for queries against Materials Project Oct 25, 2018
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Please address the in-line comments on the code. The rest of the code looks OK. It would be great if you could supply a couple of tests for the parts of the code that do not need the network access.

@espenfl
Copy link
Contributor Author

espenfl commented Oct 25, 2018

Yes, in fact I also wanted some tests for the actual pull, but then we need the key. A public test key supplied by MP would be great (e.g. just pull one demo structure for instance) but I do not know of such functionality at the moment. Right now I think the only sensible test is to test that an invalid key returns the right error message or something similar as that is already in __init__. I will add such a test for good measure.

@merkys
Copy link
Member

merkys commented Oct 25, 2018

Yes, in fact I also wanted some tests for the actual pull, but then we need the key. A public test key supplied by MP would be great (e.g. just pull one demo structure for instance) but I do not know of such functionality at the moment. Right now I think the only sensible test is to test that an invalid key returns the right error message or something similar as that is already in __init__. I will add such a test for good measure.

I guess we're OK with tests that do not interact with the API for now. We might test the generation of Results and Entry instances.

@merkys
Copy link
Member

merkys commented Oct 25, 2018

One more note on plugin naming: I suggest using full names (matp.py -> materialsproject.py, MatProj* -> MaterialsProject*) as neither matp nor matproj seem to be upstream-preferred abbreviations of the database. I admit that shorter names are faster to type, but they might be non-trivial for new users. There is actually a nice Wikipedia guidelines article on abbreviating things that is good to refer to, IMHO.

@espenfl
Copy link
Contributor Author

espenfl commented Oct 25, 2018

Yes, but I just followed what was there already. Totally agree on this topic. I also believe this should be done in the code, regardless of clutter and longer lines. Changed this now, but will leave the old stuff for now.

@merkys
Copy link
Member

merkys commented Oct 25, 2018

The importer seems to have disappeared from this PR... Could be due to rename?

@espenfl
Copy link
Contributor Author

espenfl commented Oct 26, 2018

Should be fine now. Also added the aiida.tools.dbimporters.plugins to the test folder as that was not there.

@merkys
Copy link
Member

merkys commented Oct 26, 2018

Great! It's very close to be finished. I have noted a few minor changes, and then we are ready to merge.

Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Good to go. Many thanks!

@giovannipizzi giovannipizzi merged commit cc90894 into aiidateam:develop Oct 26, 2018
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

4 participants