corrections for intialization

[monsieuralok]

In these files I made modifications for initialization of variables and I have initialized everything to zero. Please check it as sometime they do need to be initialized to zero they might have some other value.

[monsieuralok]

@oyvindseland @Kirkevag @tto061 @DirkOlivie please check this pull requesr

[DirkOlivie]

Hi Alok, after these changes, were the test-results bit-identical? Dirk

[monsieuralok]

@DirkOlivie I have not tested results after all these bugs fixed. I tested bit-wise reproducibility in between and then, I tested results in-between with and without bugs only once. They were bit-identical. But, if @tto061 and Ada has made any these tests I am not aware as they were also doing some simulation. Also, I was always executing experiment where these emissions files are not merged and I executed two simulations of 100 years and not having a single mid-month crash. If it require let me know I will do.

[tto061]

This is OK to go for me. Could reviewers please approve within the next couple of days if you can, and we can merge.

[Kirkevag]

I agree with Thomas, by the way, that the code commented out is best left as comments, for possible future use (even though it makes the code look messy).

[tto061]

Alf, for my part I already approved the pull request (and yes it was myself correcting that part; it has no consequences, bit-wise). Given that, there's no point in you or others asking the first question here.

Good for 2.; I regard all of these as bugs, because the code must not rely on arbitrary initialisations by the compiler. It is just chance that that does not cause errors in the calculations (as with all bad coding).

On 2020-06-09 11:42, Alf Kirkevåg wrote:

@Kirkevag commented on this pull request.

I've looked over the changes, which seem ok mostly. Exceptions

1. zm_conv.F90: I assume Thomas has control of the added code, wehich is not just initializion?
2. pmxsub.F90. Two variables (vnbcarr and vaitbcarr), now initialized, are also calculated, but before they are used for the first time. This is therefore a bug: the effect of this bug concerns extra diagnostics of optics at RH=0% only (for AeroCom experiments): mixture 4 and 14 has not had the right volume fraction of BC for this particular RH. The loop over higher RH values are done after calculating vnbcarr and vaitbcarr, so that seems to be OK.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#pullrequestreview-426969769, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZGLJCLUOTFYPOS6FC2Y5LRVX7WRANCNFSM4NUVK66Q.

[Kirkevag]

I just did my duty as a code reviewer, Thomas, as requested. For the second part, which of course is due to bad coding, I have now made a new github issue for this: https://github.com/NorESMhub/CAM/issues/9

[tto061]

Alf, I disagree: it was not your duty but, at the contrary, a useless and unnecessary request for a response from me.

Why?

Because it was I who nominated the reviewers, and included myself, and immediately approved the request. I did not do that to waste my time, to but save yours by making it clear what your role is. So you have now forced a response from me that I did not need to give. You have not understood that in the first place, and even now you keep insisting without apparently considering whether you're right doing so. This does not seem a good example of effective communication via github.

Maybe we can discuss in the meeting today how work together more effectively via github.

On 2020-06-09 11:55, Alf Kirkevåg wrote:

I just did my duty as a code reviewer, Thomas, as requested. For the second part, which of course is due to bad coding, I have now made a new github issue for this: #9 https://github.com/NorESMhub/CAM/issues/9

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-641178071, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZGLJGSWXPF2BNWSZPV35LRVYBKDANCNFSM4NUVK66Q.

[tto061]

just to add, for the benefit of all those who do not understand the code in zm_conv.F90: that was EXACTLY an initialisation issue, nothing more, nothing else; just like the others (only this one had no effect on the calculations, and was caught be init=zero,arrays)

[tto061]

Can we merge the first commit only now? I am not sure how to do this, by the way -- the automatic merge will take both commits. Any ideas?

[MichaelSchulzMETNO]

I found this command cherry-pick to take only one commit::

https://mattstauffer.com/blog/how-to-merge-only-specific-commits-from-a-pull-request/

On 12 Jun 2020, at 14:59, tto061 notifications@github.com wrote:

Can we merge the first commit only now? I am not sure how to do this, by the way -- the automatic merge will take both commits. Any ideas?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643256846, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD73WURD6OAX3ZBQ3USZZKDRWIRCJANCNFSM4NUVK66Q.

[oyvindseland]

I would imagine that you can also pick a specific commit from Alok's copy at the merging of the command line structure, but is there any specific reasons why you do not want both?

[tto061]

This was discussed already between me, Alok and Alf; and also indirectly at the telecon on Wednesday in the context of separating clearly cosmetic changes from substantive changes, in the interest of traceability and of easy diff'ing on the latter.

Right now we're not sure that the commented-out code cannot be useful for troubleshooting purposes in the near future, nor are we sure that that the clean-up is consistent and does not leave dangling code whose function can be hard to understand without those commented parts.

So I'd defer this to a dedicated and more comprehensive code clean-up effort in the near future.

On 2020-06-12 15:23, oyvindseland wrote:

I would imagine that you can also pick a specific commit from Alok's copy at the merging of the command line structure, but is there any specific reasons why you do not want both?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643268121, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZGLJHWGDO5W54W3MWQYLTRWIT33ANCNFSM4NUVK66Q.

[annefou]

@MichaelSchulzMETNO That was the kind of things I was talking about (or trying to) during the CAM meeting; you can update user's PR to fix problems, cherry pick, etc. on behalf of the user (so that users are rewarded for their contribution and not the maintainer).

[tto061]

OK I will try and have a go

[tto061]

sorry, I tried. Now I'm fed up. I will use git again next month, maybe.

[MichaelSchulzMETNO]

ok , I assume the first commit is fine with everybody and I will try to cherry pick just the first commit.

lets see if I manage Michael

On 12 Jun 2020, at 16:08, tto061 notifications@github.com wrote:

sorry, I tried. Now I'm fed up. I will use git again next month, maybe.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643289385, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD73WUR7HG44DOXASQNG7R3RWIZEXANCNFSM4NUVK66Q.

[tto061]

thanks, I appreciate. If you do, can you please share the git vodoo command sequence that worked for you.

:( ( <-- me=idiot )

On 2020-06-12 16:25, MichaelSchulzMETNO wrote:

ok , I assume the first commit is fine with everybody and I will try to cherry pick just the first commit.

lets see if I manage Michael

On 12 Jun 2020, at 16:08, tto061 notifications@github.com wrote:

sorry, I tried. Now I'm fed up. I will use git again next month, maybe.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643289385, or unsubscribe https://github.com/notifications/unsubscribe-auth/AD73WUR7HG44DOXASQNG7R3RWIZEXANCNFSM4NUVK66Q.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643298712, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZGLJARELAY73VZWK2HBELRWI3HHANCNFSM4NUVK66Q.

[oyvindseland]

I will go back and do some git training inbetween the finishing up of the article so I can try as well but not until next week.

[MichaelSchulzMETNO]

I will merge, then revert one commit back and then we can pick the second commit later if all agree.

[tto061]

great!

On 2020-06-12 18:07, MichaelSchulzMETNO wrote:

I will merge, then revert one commit back and then we can pick the second commit later if all agree.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643354506, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZGLJD2XFFVG4TPGAMTI7LRWJHE3ANCNFSM4NUVK66Q.

[MichaelSchulzMETNO]

Might be good to test, if this is now working. I am not sure what this part

if(micro_mg_version <2) then call post_proc%add_field(p(nctncons), p(packed_nctncons)) call post_proc%add_field(p(nctncons), p(packed_nctncons)) call post_proc%add_field(p(nctnnbmn), p(packed_nctnnbmn)) call post_proc%add_field(p(nctnnbmn), p(packed_nctnnbmn))
ENDIF

really was for.

Why did Alok introduce it in the first place? hmm

[tto061]

we did test this, but no harm in joining the big testfest before releasing. I could test on tetralith; in fact I'd also like to still add tetralith machine settings to CIME if possible.

On 2020-06-12 18:16, MichaelSchulzMETNO wrote:

Might be good to test, if this is now working. I am not sure what this part

if(micro_mg_version <2) then call post_proc%add_field(p(nctncons), p(packed_nctncons)) call post_proc%add_field(p(nctncons), p(packed_nctncons)) call post_proc%add_field(p(nctnnbmn), p(packed_nctnnbmn)) call post_proc%add_field(p(nctnnbmn), p(packed_nctnnbmn)) ENDIF

really was for.

Why did Alok introduce it in the first place? hmm

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NorESMhub/CAM/pull/8#issuecomment-643358804, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADZGLJFQNLHU4RVKTNCS2KTRWJIGBANCNFSM4NUVK66Q.