AtChem / AtChem2

Atmospheric chemistry box-model for the MCM
MIT License
58 stars 23 forks source link

Housekeeping #466

Closed rs028 closed 1 year ago

rs028 commented 2 years ago

This branch includes a number of minor updates, fixes, tweaks, and corrections made while working on the new Test Suite:

codecov[bot] commented 1 year ago

Codecov Report

Merging #466 (3773227) into master (276a826) will increase coverage by 1.27%. The diff coverage is n/a.

Additional details and impacted files [![Impacted file tree graph](https://codecov.io/gh/AtChem/AtChem2/pull/466/graphs/tree.svg?width=650&height=150&src=pr&token=4p5Cr68G8w&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem)](https://codecov.io/gh/AtChem/AtChem2/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem) ```diff @@ Coverage Diff @@ ## master #466 +/- ## ========================================== + Coverage 65.54% 66.81% +1.27% ========================================== Files 17 17 Lines 2046 2046 ========================================== + Hits 1341 1367 +26 + Misses 705 679 -26 ``` | Flag | Coverage Δ | | |---|---|---| | build | `52.81% <ø> (+0.54%)` | :arrow_up: | | tests | `67.17% <ø> (-15.95%)` | :arrow_down: | | unittests | `54.86% <ø> (+23.23%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/AtChem/AtChem2/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem) | Coverage Δ | | |---|---|---| | [src/solarFunctions.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL3NvbGFyRnVuY3Rpb25zLmY5MA==) | `90.62% <ø> (ø)` | | | [src/outputFunctions.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL291dHB1dEZ1bmN0aW9ucy5mOTA=) | `56.49% <0.00%> (+0.64%)` | :arrow_up: | | [src/inputFunctions.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL2lucHV0RnVuY3Rpb25zLmY5MA==) | `55.77% <0.00%> (+0.74%)` | :arrow_up: | | [src/dataStructures.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL2RhdGFTdHJ1Y3R1cmVzLmY5MA==) | `58.87% <0.00%> (+0.80%)` | :arrow_up: | | [src/solverFunctions.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL3NvbHZlckZ1bmN0aW9ucy5mOTA=) | `50.61% <0.00%> (+1.23%)` | :arrow_up: | | [src/constraintFunctions.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL2NvbnN0cmFpbnRGdW5jdGlvbnMuZjkw) | `42.63% <0.00%> (+2.32%)` | :arrow_up: | | [src/argparse.f90](https://codecov.io/gh/AtChem/AtChem2/pull/466/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem#diff-c3JjL2FyZ3BhcnNlLmY5MA==) | `39.84% <0.00%> (+11.71%)` | :arrow_up: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/AtChem/AtChem2/pull/466?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/AtChem/AtChem2/pull/466?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem). Last update [276a826...3773227](https://codecov.io/gh/AtChem/AtChem2/pull/466?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=AtChem).
rs028 commented 1 year ago

@spco Any comments? The idea is to work towards more consistency of the various tools, as well as naming and formatting. I am going to merge this and leave further updates to the manual and readme file for another time.

PS: can the trigger_action_on_PR_merge branch be deleted?

spco commented 1 year ago

@spco Any comments? The idea is to work towards more consistency of the various tools, as well as naming and formatting. I am going to merge this and leave further updates to the manual and readme file for another time.

PS: can the trigger_action_on_PR_merge branch be deleted?

I've now deleted this too.

spco commented 1 year ago

Looks like macos is now up and running again - I've made it a bit more robust in its handling of matrix.os so we don't have to remember to change it in multiple places.

You're right that macos-12 only supplies gfortran-11 out of the box. My concern is that our GH Actions may swiftly outpace the likely target machine for users - and HPC is far more likely to have gfortran-8 than 11! Perhaps we could consider reinstating the brew install workaround for macos-12 to install gfortran-9 and gfortran-10? TBH I think brew installing gfortran-8 on both would also be quite desirable.

rs028 commented 1 year ago

Looks like macos is now up and running again - I've made it a bit more robust in its handling of matrix.os so we don't have to remember to change it in multiple places.

You're right that macos-12 only supplies gfortran-11 out of the box. My concern is that our GH Actions may swiftly outpace the likely target machine for users - and HPC is far more likely to have gfortran-8 than 11! Perhaps we could consider reinstating the brew install workaround for macos-12 to install gfortran-9 and gfortran-10? TBH I think brew installing gfortran-8 on both would also be quite desirable.

this is my concern, and also to keep backwards compatibility but within reason, otherwise it becomes a nightmare to maintain.

spco commented 1 year ago

I'll have a play around to see what we can do without it indeed becoming a maintenance nightmare!

spco commented 1 year ago

Ok, I'm going to revert the last few commits and abandon gfortran-8 on either platform for now. The reasons:

(1) on ubuntu, gfortran-8 is no longer in the package managers (minimum supported is 9). The only alternative is to compile from source, which is possible but will take a few minutes of compute time, and I don't have time to get that working right now.

(2) on macOS, for some reason the CVODE install insists that gfortran-8 is not a functioning fortran compiler. It then deactivates the FCMIX flag, which means that the *_fcvode.a libraries are not built. It then fails later at the stage of running the atchem2 executable as it can't link to the nonexistent libraries. I'm not sure what would be required to make CVODE think gfortran-8 is functional, so I'll drop that for now too due to time.

So we should end up with ubuntu-22.04, macos-11 and macos-12, all running gfortran-9, 10 and 11.

It would still be desirable to get gfortran-8 running on at least one architecture in future, but not crucial.

rs028 commented 1 year ago

Sounds reasonable. And thanks for sorting this out. I will now try to push my last changes and (if you are done) merge this branch.

spco commented 1 year ago

Yep, I'm all done.