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

Variables related to liquid reff are not correct #53

Merged
merged 3 commits into from
Aug 13, 2020

Conversation

lqxyz
Copy link
Contributor

@lqxyz lqxyz commented Aug 12, 2020

In cosp_config.F90, the liquid Reff related variables should use LIQ rather than ICE, right?
I notice that nReffICE and nReffLIQ are equal, so the misuse of them won't cause any problems. However, reffICE_binCenters and reffLIQ_binCenters are different, which needs to be corrected.

@lqxyz lqxyz changed the title Variable names related to liquid reff are not correct Variables related to liquid reff are not correct Aug 12, 2020
@dustinswales
Copy link
Contributor

@lqxyz
Thanks for finding this bug!
May I ask why the CI tests failed? For inclusion of this PR into the master branch, these tests must pass.

@lqxyz
Copy link
Contributor Author

lqxyz commented Aug 12, 2020

The following is the error message from CI test, and it seems due to the NetCDF library for ifort.

test (ifort):

Build NetCDF FORTRAN library
checking for a thread-safe mkdir -p... /bin/mkdir -p
Run source /opt/intel/inteloneapi/setvars.sh || true
/home/runner/work/_temp/ac9c91f0-2733-494f-aa2a-3a9712fbcd5a.sh: line 1: /opt/intel/inteloneapi/setvars.sh: No such file or directory

@alejandrobodas
Copy link
Collaborator

I've compared the outputs of the ifort step with a previous successful test, and it looks like the CI test retrieves the latest ifort version, and therefore we can be affected by changes in the ifort configuration (e.g. /opt/intel/inteloneapi/setvars.sh doesn't exist in the latest ifort or it's in a different place).
This is a problem of robustness of the CI test that we need to fix.

@alejandrobodas
Copy link
Collaborator

I have looked at the Intel OneAPI documentation, and it seems that setvars.sh is now located in /opt/intel/oneapi. However, changing this doesn't fix the ifort test, it still fails with an error in configure:
configure: error: Could not link conftestf.o and conftest.o
I have no idea why this happens. I'll create an issueto document this.

@alejandrobodas
Copy link
Collaborator

@lqxyz I've fixed the CI tests in #55 . You should be able to continue with the development/testing of this change.

@lqxyz
Copy link
Contributor Author

lqxyz commented Aug 13, 2020

Thank you very much @alejandrobodas. I wonder how could I restart the CI test for this P/R?

@RobertPincus
Copy link
Contributor

RobertPincus commented Aug 13, 2020 via email

@lqxyz
Copy link
Contributor Author

lqxyz commented Aug 13, 2020

Thank you @RobertPincus. I have merged my branch with the master as you suggested, and now all the CI tests have passed. It looks like this P/R can be merged into master after some reviews.

@RobertPincus
Copy link
Contributor

@dustinswales It looks good to me but maybe you can check and formally approve? This is really in the guts of the thing...

@dustinswales dustinswales merged commit c578370 into CFMIP:master Aug 13, 2020
@dustinswales
Copy link
Contributor

I approve of these changes.

@lqxyz lqxyz deleted the modis_reff_liq branch August 14, 2020 06:42
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.

None yet

4 participants