Skip to content

Replace Fortran Disort with C Disort - #80

Merged
olemke merged 11 commits into
atmtools:masterfrom
olemke:cdisort
Sep 30, 2019
Merged

Replace Fortran Disort with C Disort#80
olemke merged 11 commits into
atmtools:masterfrom
olemke:cdisort

Conversation

@olemke

Copy link
Copy Markdown
Member

This PR adds cdisort version 2.1.3 to the 3rdparty directory in ARTS.

The WSM DisortCalc now uses cdisort by default. For testing purposes, reverting to Fortran Disort is supported via a generic argument to DisortCalc.

The disort ARTS code has been cleaned up and all unused functions, as well as the experimental DisortCalcWithARTSSurface, have been removed.

@olemke

Copy link
Copy Markdown
MemberAuthor

@erikssonpatrick Is it okay with you to remove DisortCalcWithARTSSurface? Are you aware of anyone using it currently? @m-brath proposed do a new implementation at some point using the BDRF functionality available in cdisort.

@m-brathm-brath left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FORTRAN code should be removed after final testing before merging.

@olemkeolemke added the in progress Features that are still under development label Sep 23, 2019
@erikssonpatrick

Copy link
Copy Markdown
Contributor

@olemke@m-brath A good question. The surface is an issue for most of our scattering solvers! My opinion is that the scattering solvers should consider surface_rtprop_agenda and that the surface can be placed at any altitude. I assume DisortCalcWithARTSSurface was an attempt to move in this direction. Despite this, I say let's remove DisortCalcWithARTSSurface. I think it is best to start from scratch.

Can you inform me about how the surface is treated right now in the cdisort version? Whatever you have done, it is OK for now. But feel free to make an issue on improving the integration of Disort and RT4 with respect to how ARTS defines the surface properties. That would be something for me if I just find the time ...

@erikssonpatrickerikssonpatrick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my comment.

@olemke

Copy link
Copy Markdown
MemberAuthor

Currently, the surface is treated identical in C and Fortran Disort. It uses a lambertian bottom boundary with surface_scalar_reflectivity as the albedo.

@erikssonpatrick

Copy link
Copy Markdown
Contributor

@olemke As I wrote, further work on the surface inside cdisort should be a new issue. But maybe this could be quickly done: If my memory is correct, cdisort can do both lambertian and specular surface. Correct? If yes, could you just add a gin to select between the two options? Then using surface_scalar_reflectivity for both options.
Specular is what is assumed normally for microwave simulations, so would be good to have that option at hand.

@olemke

Copy link
Copy Markdown
MemberAuthor

CDisort supports lambertian and different types of BRDF. I think if I turn off lambertian and set BRDF_TYPE to None, it should result in a specular surface.

@erikssonpatrick

Copy link
Copy Markdown
Contributor

@olemke You wrote, "I think". If you are uncertain, can you point me to some relevant documentation and I maybe I can give a piece of advice? This would be good to know anyhow.

@olemke

Copy link
Copy Markdown
MemberAuthor

Thanks @erikssonpatrick. I'm indeed not certain, mostly due to the lack of documentation. :-( Except for the paper, everything I could find documentation-wise, is included in the source directory. This example in disotest.c seems to do non-lambertian without special BRDF, but is it really specular? Maybe you can extract some helpful information from DISORT2.doc.

@erikssonpatrick

Copy link
Copy Markdown
Contributor

@olemke My conclusion is that there are two options for the surface. The simplest option is ds.flag.lamber = TRUE; This option should be identical to what we have in the fortran version. So this is OK for the moment.
Second option: Other surface models are found in c_bidir_reflectivity. I assume setting ds.flag.lamber = FALSE; activates this function, but I have not checked the details. That is, it should hopefully be possible to link cdisort to surface_rtprop_agenda by c_bidir_reflectivity.
In summary, no simple fix right now. You have done your part. OK for me to accept the pull request.

@olemke

Copy link
Copy Markdown
MemberAuthor

@erikssonpatrick, that fits with @m-brath and my conclusion.

@m-brath and I want to do one more test of cdisort tomorrow with a realistic scenario. If that is successful, I'll remove the Fortran version and merge the PR.

@olemkeolemke removed the in progress Features that are still under development label Sep 30, 2019
@olemke

Copy link
Copy Markdown
MemberAuthor

The tests have shown that cdisort is in good agreement with the Fortran version. Thus, the Fortran code has been removed.

@olemke
olemke merged commit c408040 into atmtools:masterSep 30, 2019
@olemke
olemke deleted the cdisort branch September 30, 2019 12:49
@olemkeolemke mentioned this pull request Oct 1, 2019
Sign up for freeto 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.

3 participants

@olemke@erikssonpatrick@m-brath