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

add index-metadbs/matcloud #63

Merged
merged 3 commits into from
Mar 2, 2021
Merged

Conversation

jpdong00
Copy link
Contributor

@jpdong00 jpdong00 commented Mar 2, 2021

Based on your opinion, the static index meta-databases has been added.
And for some irregularities, I will carefully check and modify.
Thank you for your suggestion.

@ml-evs
Copy link
Member

ml-evs commented Mar 2, 2021

Thanks for addressing our suggestions @jpdong00, the index meta-db you have added looks fine to me. Could you please keep all changes in one pull request? You can just push new changes to the master branch of your fork, and it will automatically update this PR. It's quite hard to track what's going on otherwise!

I've copied @CasperWA's previous review comments from the previous PR.

"attributes" : {
"name": "An integrated high-throughput computational materials platform",
"description": "MatCloud is a high-throughput computing platform integrating data, simulation and supercomputing.",
"base_url": "http://207.246.86.9:5000/optimade",
Copy link
Member

Choose a reason for hiding this comment

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

You don't have a named domain to put in here instead? As otherwise you will have to change this again later once you get that.

Furthermore, The OpenAPI schema shown there does not reflect what would be expected from the named endpoints. See the specification here to understand better how to setup your OPTIMADE implementation (e.g., endpoint names should consist of lowercase alphanumerics and usually be in the plural form, e.g., /structures instead of /Structure).

Originally posted by @CasperWA in #61 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Our website is still under construction, and only some of the functions of the optimade specification have been implemented. And for these irregularities, I will carefully check and modify.

Later, the domain name will be unified under MatCloud (http://matcloud.cnic.cn/).
Thank you for your suggestion.

Originally posted by @jpdong00 in #61 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Just to add my two cents here, I would suggest setting your base_url here to null for now, until you have finished your implementation. This will reserve the appropriate prefix and signal the intent of matcloud to implement OPTIMADE, but it will prevent your database being used by all OPTIMADE clients/validators until it is finished.

You may be interested in using the validator we have written at (https://github.com/Materials-Consortia/optimade-python-tools) to help finish off your implementation. It is also available as GitHub action (https://github.com/Materials-Consortia/optimade-validator-action) and as a no-install binder repository (https://mybinder.org/v2/gh/ml-evs/optimade-validator-binder/master?filepath=optimade_validator.ipynb) where you can test your current URL.

@jpdong00
Copy link
Contributor Author

jpdong00 commented Mar 2, 2021

Thanks for addressing our suggestions @jpdong00, the index meta-db you have added looks fine to me. Could you please keep all changes in one pull request? You can just push new changes to the master branch of your fork, and it will automatically update this PR. It's quite hard to track what's going on otherwise!

I've copied @CasperWA's previous review comments from the previous PR.

Thank you again for your patience and guidance @ml-evs , please change the base_url to null.
I pushed the new changes to the master branch. Is there a problem with my operation? I am a rookie about git, could you give me some advice about this

Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I don't think I have push access to your branch, but I have made the change here as a commit suggestion.

src/index-metadbs/matcloud/v1/links.json Outdated Show resolved Hide resolved
@ml-evs ml-evs self-requested a review March 2, 2021 17:18
ml-evs
ml-evs previously approved these changes Mar 2, 2021
Copy link
Member

@ml-evs ml-evs left a comment

Choose a reason for hiding this comment

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

I think this can now be accepted as it these changes just reserve the matcloud prefix and indicate that a MatCloud OPTIMADE implementation is in progress.

Once you have a URL that hosts the OPTIMADE API, @jpdong00 please make another PR to update it here. Hopefully you will find the other tools I linked above useful, good luck!

@CasperWA do you want to double-check this, as the other person who initially reviewed it?

Copy link
Member

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Found a single change that should be implemented, and then it's good to go.

Perhaps this is even a change that needs to go into the template?
Edit: Just checked. The template is fine.

src/index-metadbs/matcloud/v1/info.json Outdated Show resolved Hide resolved
@CasperWA CasperWA merged commit e2074e8 into Materials-Consortia:master Mar 2, 2021
@jpdong00
Copy link
Contributor Author

jpdong00 commented Mar 3, 2021

@ml-evs Thank you very much, your help is a great encouragement to us. There may be some irregularities now, we will proofread again and continue to implement our website in accordance with the optimade specifications.

In addition, there is a small question, how can we display our information on this website (https://www.optimade.org/providers-dashboard/)?

1 similar comment
@jpdong00
Copy link
Contributor Author

jpdong00 commented Mar 3, 2021

@ml-evs Thank you very much, your help is a great encouragement to us. There may be some irregularities now, we will proofread again and continue to implement our website in accordance with the optimade specifications.

In addition, there is a small question, how can we display our information on this website (https://www.optimade.org/providers-dashboard/)?

@CasperWA
Copy link
Member

CasperWA commented Mar 3, 2021

In addition, there is a small question, how can we display our information on this website (https://www.optimade.org/providers-dashboard/)?

That website is automatically updated once a day based on the providers in this repository. So as soon as you don't have a base_url that's null, you will start to see more information. But you're already on there for now: https://www.optimade.org/providers-dashboard/providers/matcloud.html

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

3 participants