search

BoothGroup / Vayesta

A Python package for wave function-based quantum embedding
Apache License 2.0
32 stars 7 forks source link

Pr/doc adjustments #25

Closed maxnus closed 2 years ago

maxnus commented 2 years ago

Small adjustments to the documentation

@obackhouse @alejandrosantanabonilla I added the docs/source/apidoc directory for sphinx-apidoc files, since I think it's good to separate automatically created files in their own directory. I changed the call in workflows/ci.yaml to sphinx-apidoc -o source/apidoc -t templates ../vayesta/ is this all that needs to be done?

@ghb24 I reverted/changed some minor things of your corrections, in case you want to have a look

obackhouse commented 2 years ago

is this all that needs to be done?

yes should be fine - I think actually the changes will be deployed directly from this PR once the CI gets past installation and tests so you can double check online before merging. That's a bad idea but probably fine for now because it means we can preview the changes made by PRs. I think later we need to change

https://github.com/BoothGroup/Vayesta/blob/522da84dbe886dde38211d37b21bb41b2d32e9cc/.github/workflows/ci.yaml#L56

to 

    if: matrix.documentation && github.ref == 'refs/heads/master'
alejandrosantanabonilla commented 2 years ago

Let's waut for the tests to finish and see if the passed, including creating correctly the Docs. @obackhouse , the github.ref is for running the docs only for the master, right? Maybe, the branches workflow keyword might be useful here?

on: push: branches:

This is running all tests but in the master one. If not, then we can just leave the github.ref, might be easier to follow the .yml file.

maxnus commented 2 years ago

I think actually the changes will be deployed directly from this PR once the CI gets past installation and tests so you can double check online before merging. That's a bad idea but probably fine for now because it means we can preview the changes made by PRs.

However, if there are multiple PRs, the latter will overwrite the docs of the former? Can we have two worksflows, once for the master branch deploying the main documentation, and another one for PRs, deploying to a PR specific link?

obackhouse commented 2 years ago

Let's waut for the tests to finish and see if the passed, including creating correctly the Docs. @obackhouse , the github.ref is for running the docs only for the master, right? Maybe, the branches workflow keyword might be useful here?

on: push: branches: - '*' - '!master'

This is running all tests but in the master one. If not, then we can just leave the github.ref, might be easier to follow the .yml file.

not sure what you mean here, but the current ci.yaml runs on pushes and PRs to master branch, but we only want the docs to deploy on pushes to master (and not PRs). We could of course have a separate workflow for the docs with a separate schedule but it is nice to have them combined. But that's easily controlled by the change I stated above.

EDIT: ok I get you - the on: block controls the entire workflow, but the if: blocks can control individual steps of the workflow.

obackhouse commented 2 years ago

However, if there are multiple PRs, the latter will overwrite the docs of the former? Can we have two worksflows, once for the master branch deploying the main documentation, and another one for PRs, deploying to a PR specific link?

yeah, that's one issue. I don't think you can have two on the same repo but I suppose we could open a new repo for testing the deployment. I think I could then set up a workflow in this main repo that deploys onto the testing repo without having to PR to the testing repo manually - if you think it's worth the effort

alejandrosantanabonilla commented 2 years ago

Indeed, that is the point. So the PRs and pushes for all branches except master are carried out by this command. So, I think this command could be used to fork the PRs and push events using the different branches. we can split the workflow and say, all PRs and push events of all other branches will not build the docs. The PRs and Push events of the docs are only built using master....

obackhouse commented 2 years ago

Indeed, that is the point. So the PRs and pushes for all branches except master are carried out by this command. So, I think this command could be used to fork the PRs and push events using the different branches. we can split the workflow and say, all PRs and push events of all other branches will not build the docs. The PRs and Push events of the docs are only built using master....

yes, that is what my suggested change does

alejandrosantanabonilla commented 2 years ago

alright, so i understood correctly what you wanted to do. We can have also the option of the branches, giving us the freedom to decide what to push for the different branches...

maxnus commented 2 years ago

yeah, that's one issue. I don't think you can have two on the same repo but I suppose we could open a new repo for testing the deployment. I think I could then set up a workflow in this main repo that deploys onto the testing repo without having to PR to the testing repo manually - if you think it's worth the effort

I think for the time being we can just prevent PRs from deploying the docs, if thats easier

obackhouse commented 2 years ago

yeah, that's one issue. I don't think you can have two on the same repo but I suppose we could open a new repo for testing the deployment. I think I could then set up a workflow in this main repo that deploys onto the testing repo without having to PR to the testing repo manually - if you think it's worth the effort

I think for the time being we can just prevent PRs from deploying the docs, if thats easier

Fine by me

maxnus commented 2 years ago

Fine by me

Could you add that change? Thanks

obackhouse commented 2 years ago

Could you add that change? Thanks

done, please just double check that it all passes before merging

maxnus commented 2 years ago

So the onlien docs atm don't seem quite right: in https://boothgroup.github.io/Vayesta/quickstart/ewf.html the code snippets are missing - in the DMET quickstart they are present. Are these docs generated from the master branch or this PR? If I build them locally from either branch, the code snippets are present

alejandrosantanabonilla commented 2 years ago

The docs should be created from this PR, since it is not merged yet. I think the problem is the relative path we are using for referencing the .py files. In ewf, we have the following path:

/../../../vayesta/example/....

whereas in edmet, we have:

/../../examples/ewf/molecules/01-simple-ccsd.py

I remember I made a comment to be merged in tutorials for changing this relative path. In ay case, it is easy to change, we need to change all the relative paths in ewf to the latter form and the correct file will be called.

obackhouse commented 2 years ago

The docs won't be deployed from this PR since https://github.com/BoothGroup/Vayesta/pull/25/commits/8261df40c2d8d984355acea618e3a51d71035651 but they will have been deployed from earlier pushes. The most recent deployment is https://github.com/BoothGroup/Vayesta/commit/a84b244922a84c3695044890e4d1be43c93014c1.

alejandrosantanabonilla commented 2 years ago

I have changed the relative paths in the main/docs folder. I have commited those changes. This should solve the problem

maxnus commented 2 years ago

The docs should be created from this PR, since it is not merged yet. I think the problem is the relative path we are using for referencing the .py files. In ewf, we have the following path:

/../../../vayesta/example/....

whereas in edmet, we have:

/../../examples/ewf/molecules/01-simple-ccsd.py

I remember I made a comment to be merged in tutorials for changing this relative path. In ay case, it is easy to change, we need to change all the relative paths in ewf to the latter form and the correct file will be called.

Ah well spotted - the repository directory is likely named Vayesta rather than vayesta (which is what I named it locally), which is why it worked locally for me