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

New OptaDOS Photoemission Module #57

Open
wants to merge 197 commits into
base: develop
Choose a base branch
from

Conversation

wuppersaver
Copy link

This is a pull request to integrate the new photoemission module into the OptaDOS development version.

Merged the elements that differed in optados_mod_working by hand to
ensure functionality was retained. Untested!
Changed minor things in cell.f90 to allow the photoemission to work
The Calculation of epsilon_2 is cell volume dependent. This was changed,
so that epsilon_2 uses photo_slab_volume, which should lessen the
cell volume dependence of the QE values.
Preliminary data suggests otherwise, so this is a WIP
Merged 'Incorrectly Commited Changes into Development Branch'
Changed the condition from equal to zero to less than tiny number for
what kind of formula is used to compute the ARPES Phi angle
Implementing the use of user supplied imfp values for mixed material
systems
Added statements to use the correct volume (i.e. photo_slab_volume in
the case dos_per_volume during a photoemission run is enabled.
Added the option to turn off the extra output files and made it the
default behaviour.
Code in dos_utils.f90 checked for uninitialised variables since they
belong to other smearing methods. Changed the default efermi setting to
rectify the parameters printing function.
Changed printing in photo.f90 to make function calls clearer
* Create docs_to_pages

* Rename docs_to_pages to docs_to_pages.yml

* Update docs_to_pages.yml

* Update docs_to_pages.yml

* Update docs_to_pages.yml
* Create docs_to_pages

* Rename docs_to_pages to docs_to_pages.yml

* Update docs_to_pages.yml

* Update docs_to_pages.yml

* Update docs_to_pages.yml

* Ran pre-commit

* Update main.yml

Made sure that optados.x is created to run the tests in the test-suite

* Update main.yml

Made sure optados.mpi is created and available for the test-suite

* Update main.yml

* Corrected .gitignore files

* Reformulated and corrected qe_mat mpi test
@ajm143
Copy link
Contributor

ajm143 commented Jul 20, 2023

@wuppersaver Thanks very much for this. I can see that you've added to the test suite, so that's really helpful. I'm back from SPL now, so hopefully can get this compiled up and tested, and will get back to you.
Best wishes,
Andrew

@wuppersaver
Copy link
Author

Hi @ajm143 ,
that sounds very good. I am not entirely done with what I wanted to achieve before the merge, but the outstanding tasks are almost all updating the user guide tex files and revisiting the examples. I have introduced quite a few changes since the version when they were created, so I would like to add some, that are more representative of what the code can do. I would say the functionality and the source code are at a point, where I won't have to change anything unless you request any changes.

I added the Docs to Pages task to test out the automatic making of the FORD documentation and to upload it to a GitHub pages on my personal fork. Since there is a different setup, it won't work here and I can remove it, too.

How strict are you right now on the FORD annotations? I have found that the created FORD docs are not entirely correct for the photoemission module and I would have to familiarize myself with FORD a bit more to clean up the docs for the photo.f90 file.

Best,
Felix

@ajm143
Copy link
Contributor

ajm143 commented Jul 20, 2023

Hi Felix (@wuppersaver),

I think it makes sense for me (and probably @jryates) to review the code now, just in case there are things that we'd like changing.

Ford: You're ahead of me in terms of GitHub integration. So rather than removing it, what's involved in modifying it make the docs for the official repo?

As you can see, we're retrofitting FORD to the code, and I haven't gotten very far with annotating the rest of the source. However, I'd like the new code additions to be as well commented as possible -- especially as the ODG haven't written them, but might have to tweak them in the future!

Best wishes,
Andrew

@wuppersaver
Copy link
Author

Hi Andrew (@ajm143 ),

sounds good on the review part.

I have only just started looking into the deployment of github pages and how ford interacts with it, so I will think again on how to set all things up properly and get back to you probably tomorrow.

I have also thought of ways to make the user guide easier to maintain than a single latex file to copy/paste into. In the research group people have used sphinx to create documentation for their python projects, but I know that there are modules for shpinx and fortran. Given that we are using FORD already, I don't want to redo work unnecessarily, but for now I don't exactly know how to add something like a user guide to the FORD documentation that is implemented so far. One example I found is at https://stdlib.fortran-lang.org/#fortran-stdlib-api-documentation

I will also start to write FORD documentation for the photoemission code, but it might take me a bit to get up to speed on this.

Let me know what you think and I will get back to you with more specific instructions on the GitHub integration steps.

Best wishes,

Felix

@ajm143
Copy link
Contributor

ajm143 commented Jul 20, 2023

Hi Felix,

It think in terms of the user guide (.tex), please just update with the photoemission info it in the user guide's current style. At a later date (version 3.0?) I'll work on how to make it interface with FORD, and probably get rid of the latex altogether. But I don't think that's for today.

At the moment, if we can just get FORD comments included in the photoemission contribution and as a bonus, get the FORD it generates onto the web somewhere, I'd be satisfied.

Best wishes,
Andrew

@wuppersaver
Copy link
Author

Hi Andrew - @ajm143,

I have had a look and the steps should be as follows:

  1. create a branch in the repo named gh-pages (this could be any name. I have set it up with the default settings, that are using this name) It should be fine to have just a copy of the repo in there for the first step as this will be overwritten later
  2. go into settings - pages and choose to create a GitHub pages page for the repository. You should be able to choose the branch it is based off (should be set to gh-pages) and choose the root folder
    image
  3. run the action that is defined in the docs_to_pages.yml file (here - as it is already part of the repository, you should be able to just call the action manually from the actions tab
    According to the author of one of the actions I used, it is normal that it will fail the first time around to deploy the page to the GitHub pages page. Rerunning the action should result in a successful deployment on the second try.

I have set up the action to run every time there is a new pull request or a commit to the develop branch to automatically update the documentation version on the pages. That can be changed, however.

I hope that I didn't forget anything. Best wishes,

Felix

@ajm143
Copy link
Contributor

ajm143 commented Jul 28, 2023

nagfor -c -DNAG -O3 -Oassumed -w=all  -ieee=full photo.f90
NAG Fortran Compiler Release 7.1(Hanzomon) Build 7108
Error: photo.f90, line 155: Missing comma in format specification
[NAG Fortran Compiler error termination, 1 error]
make[1]: *** [Makefile:90: photo.o] Error 2
make[1]: Leaving directory '/u/rscratch/ajm255/src/optados_photo/optados_photo_dev/optados/src'
make: *** [Makefile:13: optados] Error 2

This does not compile with the nag compiler. If you don't have access to nag to test, I'm happy to recompile an updated PR.

@ajm143
Copy link
Contributor

ajm143 commented Jul 28, 2023

gfortran
ifort
AOCC
oneAPI
pgf90
All are happy with the code.

Removed extra character in formatting description
@wuppersaver
Copy link
Author

Hi Andrew,

it is great to hear that only NAG had something to complain,. I got access to NAG and have changed the character that was messing with it, as well as successfully compiled the code using NAG.

On the note of the pages. It could be, that the deployment of the pages is failing due to lack of writing rights. Unfortunately I don't have much experience with how to write to secured branches and don't know what kind of security you have on the different branches. I have no security on my gh-pages branch, so that might be the reason rerunning the action, hasn't worked so far.

Best wishes,

Felix

@ajm143
Copy link
Contributor

ajm143 commented Sep 1, 2023

Just a note: I think the Docs to Pages check is failing because its in a branch and cannot write to the webpage. I'm hoping it will be fixed when we accept the PR. If not, I'll dig further.

@ajm143
Copy link
Contributor

ajm143 commented Sep 1, 2023

Dear Felix @wuppersaver ,

Something we've done seems to have broken 4 of the tests after compilation. Please could you take a look?

Best wishes,
Andrew

@wuppersaver
Copy link
Author

Dear Andrew @ajm143,

I have checked over the tests and realised I did not update some of the benchmark files, after finding a off-by-one-error in one of the calculations during the run, that slightly changes the values. I have now updated the benchmark files and I verified that the tests pass again on my own machine. Apologies for forgetting this.

Best wishes,
Felix

@wuppersaver
Copy link
Author

Just to add to this - we have recently realised, that the way we calculate the photo-emission values within the photo.f90 module are incorrect. I have been recently working on resolving the issues here and made good progress, but there is some more work to be done. That is why I have not been able to get much further on the documentation and user documentation.

The rework of those parts will require a couple of changes within the interface between photo.f90 and optics.f90 and further ones within the photo-emission module, but no major rework of the structure. If you already have some comments, I am happy to work those in to the 'correct' module.

Many thanks in advance and sorry for having to backtrack slightly - this only became clear recently.

@ajm143
Copy link
Contributor

ajm143 commented Sep 13, 2023

Dear Felix @wuppersaver

Ok. Thanks for letting us know. As you imply, I think we should carry on with the review, and worry about the minor changes necessary later.

@jryates Are you able to help with the code review?

Best wishes,
Andrew

@ajm143
Copy link
Contributor

ajm143 commented Nov 28, 2023

Hi Felix (@wuppersaver)

I realise we haven't made much progress at our end. Do you have anything to report from yours?

Best wishes,
Andrew

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