materialsproject / api

New API client for the Materials Project
https://materialsproject.github.io/api/
Other
107 stars 39 forks source link

Update chemenv documetation and suggestions #771

Closed JaGeo closed 1 year ago

JaGeo commented 1 year ago

@munrojm , I had a quick look.

I think, it would be useful to be able to search for elements and species as well (for example, if you would like to search for new Li ion conductors). In addition, I very much like chemenvsymbol as well as this is directly linked to the ChemEnv implementation. Could we also make it more clear which kind of options for chemenv... exist? I added a bit in the documentation but as I defined the Literals maybe more options exist.

Let me know what you think.

I also found some tiny typos in the documentation.

munrojm commented 1 year ago

Absolutely, very happy to add that. I'll push the changes here and we can iterate.

munrojm commented 1 year ago

@JaGeo, I have updated the client to be commensurate with the changes in https://github.com/materialsproject/emmet/pull/703.

Once deployed we can merge this if you are okay with the changes.

JaGeo commented 1 year ago

@munrojm , thank you. I will then test it after the deployment has happened. I now just made a tiny change on the documentation.

JaGeo commented 1 year ago

@munrojm let me know if I can do anything more. In any case, thank you for all the work!

munrojm commented 1 year ago

No worries! We just deployed, so I wanted to add tests and get everything to pass before pinging you to take a look before we merge. I should also point out that we have a preliminary table on the details pages showing some of the data. Here is an example. Any changes or additions you would like to make are very welcome.

munrojm commented 1 year ago

@JaGeo Actually, I am just realizing that this PR won't have tests pass because of API key issues. Everything passes when running locally, so I would just take a look and test the client now with the deployed API. If you are okay with everything, we can merge. Happy to iterate too.

JaGeo commented 1 year ago

@munrojm To me, it looks good! I think you can now query everything important for the environments. Thanks for this!

munrojm commented 1 year ago

@JaGeo Sounds good!