optados-developers / optados

Official Repository of the Optados code
http://www.optados.org
20 stars 22 forks source link

New OptaDOS Photoemission Module #57

Open wuppersaver opened 1 year ago

wuppersaver commented 1 year ago

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

ajm143 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago
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 commented 1 year ago

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

wuppersaver commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

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