search

CFMIP / COSPv2.0

COSP - The CFMIP (Cloud Feedbacks Model Intercomparison Project) Observation Simulator Package
41 stars 38 forks source link

Variables related to liquid reff are not correct #53

Closed lqxyz closed 3 years ago

lqxyz commented 3 years ago

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.

dustinswales commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

lqxyz commented 3 years ago

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

RobertPincus commented 3 years ago

Merge the changes from branch master onto your branch. This will require a commit, which will fire up another round of CI.

lqxyz commented 3 years ago

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 commented 3 years ago

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

dustinswales commented 3 years ago

I approve of these changes.