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

Sce branch #101

Merged
merged 40 commits into from
Sep 13, 2023
Merged

Sce branch #101

merged 40 commits into from
Sep 13, 2023

Conversation

yiqunyu
Copy link
Contributor

@yiqunyu yiqunyu commented Jul 21, 2022

Enabled self-consistent electric field: RSCE, as a new option for the Electric Field.

Precipitation flux is firstly calculated, and passed to electric potential solver, in which the Robinson formula is used to obtain height-integrated conductance or the GLOW model is used to calculate height-dependent conductivity. The latter is parallelized. The GLOW model is added in "srcGlow".

@drsteve
Copy link
Collaborator

drsteve commented Jul 25, 2022

Thanks @yiqunyu ! We'll take a look as soon as we can...

The first thing I notice is that there's no test case for this, nor are there any unit tests. Can I ask you to at least throw in a regression test so that we can tell when we change and/or break the behaviour! That'll help us look over and test this too (as it'll also give us a PARAM file for using SCE).

@Pheosics
Copy link
Collaborator

Just some basic questions as I start to look at this. I see the GLOW model is included, do we know what the license used by them is? We will want to make sure it doesn't conflict with ours. Also, is the GLOW model added the same as the release version, or did you have to modify it in any way to make it compatible with RAM-SCB?

@yiqunyu
Copy link
Contributor Author

yiqunyu commented Jul 27, 2022

Thanks @yiqunyu ! We'll take a look as soon as we can...

The first thing I notice is that there's no test case for this, nor are there any unit tests. Can I ask you to at least throw in a regression test so that we can tell when we change and/or break the behaviour! That'll help us look over and test this too (as it'll also give us a PARAM file for using SCE).

Sure! I will set up the test soon. @drsteve

@yiqunyu
Copy link
Contributor Author

yiqunyu commented Jul 27, 2022

Just some basic questions as I start to look at this. I see the GLOW model is included, do we know what the license used by them is? We will want to make sure it doesn't conflict with ours. Also, is the GLOW model added the same as the release version, or did you have to modify it in any way to make it compatible with RAM-SCB?

The GLOW model is downloaded from HAO: https://www2.hao.ucar.edu/modeling/glow/code. The version I added is the most recent one (0.982). I had to modify the subroutine glowbasic.f90, which is the main program for running the model, to couple with RAM-SCB.

I just noted that I missed the Glowlicense.txt and Glow.txt while uploading. I will add them soon together with the test setting. The license information is also on their website as mentioned above, if you'd like to have a quick check.

@yiqunyu
Copy link
Contributor Author

yiqunyu commented Jul 27, 2022

Just some basic questions as I start to look at this. I see the GLOW model is included, do we know what the license used by them is? We will want to make sure it doesn't conflict with ours. Also, is the GLOW model added the same as the release version, or did you have to modify it in any way to make it compatible with RAM-SCB?

The GLOW model is downloaded from HAO: https://www2.hao.ucar.edu/modeling/glow/code. The version I added is the most recent one (0.982). I had to modify the subroutine glowbasic.f90, which is the main program for running the model, to couple with RAM-SCB.

I just noted that I missed the Glowlicense.txt and Glow.txt while uploading. I will add them soon together with the test setting. The license information is also on their website as mentioned above, if you'd like to have a quick check.

One more note :-) I started modifying the GLOW model in 2016, but some newer versions got released in 2017 and 2018. So I incorporated the changes with the newer versions. Should I note the change date as 2016 still or later?

@Pheosics
Copy link
Collaborator

Looking at 3b of their license (which I am guessing is why you are asking about the date thing) I would say you should put todays date or whatever the date of the last merge request into RAM ends up being.

I'm a little concerned about section 8 of their license ("Export Law Assurances. All Software and any technical data delivered under this Agreement are subject to U.S. export control laws and may be subject to export or import regulations in other countries.") But I also don't understand how that can be true since the code is available open source on GitHub. So we probably don't need to worry about it.

@yiqunyu
Copy link
Contributor Author

yiqunyu commented Aug 16, 2022

@drsteve @Pheosics
Hi Steve and Miles, I have done with the updates on self-consistent electric field. I also added a PARAM.in.testSCE, modified from the default PARAM.in by adding some SCE-related commands. Please take a look and let me know if anything is inappropriate. Thanks!

@drsteve
Copy link
Collaborator

drsteve commented Mar 24, 2023

Sorry we haven't finished looking at this yet @yiqunyu, it's a big job and we've both had other pressures on our time when it comes to RAM-SCB work.

As there had been some other changes to the master branch I went ahead and rebased the changes onto this branch, so everything was in sync. I've also made a small change to the Makefile so that the SCE test will run. I'm seeing small differences in the output from the reference result you uploaded, so we'll probably need to update those so that the test passes (my guess is that it's the same small platform-dependent changes we see in other tests, generally differences around 1e-4 or less). If @Pheosics can grab the updated branch and run the test (make testSCE) that would be helpful. I'm also going to figure out how much runtime we can realistically use for our tests on GitHub Actions, and then maybe add a couple more tests to the CI suite.

As soon as there's a path forward on having tests that pass I think I'll merge this. At that time I'll also open an issue to remind us that we have multiple versions of dependent libraries (e.g., IRI) and we should rationalize that.

- test only runs 10 minutes
- reference files were oddly formatted (hard wrap at ~80 cols)
- reference log ran through 30 minutes (plus very minor diffs)
- reference pressure was taken from minute 15
- update compares pressure at minute 10 and uses the truncated log
@drsteve drsteve merged commit 200cef0 into lanl:master Sep 13, 2023
1 check passed
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.

3 participants