search

compdyn / partmc

Particle-resolved stochastic atmospheric aerosol model
http://lagrange.mechse.illinois.edu/partmc/
GNU General Public License v2.0
27 stars 15 forks source link

Checksum mock_monarch multiple cells (fix) #100

Closed cguzman95 closed 4 years ago

cguzman95 commented 4 years ago

Seems after the merge the calculation of multiple cells and 1 cell consecutively on mock_monarch is not working. MOST PROBABLY some variable needs a correct reset after the execution of one case.

Fix this and the checksum will ensure if the results are correct

mattldawson commented 4 years ago

hi @cguzman95 - is this still failing? or have the recent modifications fixed it? I agree with adding a check in the test to do the comparison so that it fails if the single and multi-cell results don't match

cguzman95 commented 4 years ago

Let me fix it in chem_mod properly - It's only a part of code that I commented to you some days ago (the one of camp_state and grid_offset). I don't know why is giving problems with this comprobation, but the fix is very stupid

In other order of things, this comprobation should be improved, but this should be another issue for distant future

cguzman95 commented 4 years ago

Hi @mattldawson - Pull request created. Some notations related but away from this issue.

Monarch_2 is failing on this checksum, but I think is cause the bug we commented of some reaction making multi-cell give wrong results

This type of issue won't see in unit_test since mock_monarch doesn't create an extra and independent class for multi-cell: It reset the class created with the n_cells configuration. I think this is fine to test methods like "initialize" and "destroy" from classes work correctly, but change it or not can be commented apart.

After you accept the pull-request, close this issue

mattldawson commented 4 years ago

Hi @cguzman95 - sorry, I forgot to mention that I left this off the last pull request because I wanted to look into it more.

I understand that this fixes the problem, but we need to know why because other parts of the model could be affected by the same problem.

It turns out (after some googling) that fortran, in its infinite wisdom, secretively and behind-the-scenes, changes local function variables to static if you initialize them with a value when you declare them. (This is crazy to me because fortran already has a save keyword to make a variable static, and how would this ever be useful to anyone? and why would you encourage people to use static variables?!?). But, it explains why this fixed your problem. It also explains why fortran is a dead language.

I looked through the rest of the code for similar situations, and fortunately there were only a few and most of them wouldn't affect the results because they always get overwritten or they stay constant throughout the run). But, there was one reaction that we don't use much yet, that under certain conditions might be affected by this. So, it's good we fixed it.

I'll merge the branch, and we can put the multi/single cell comparison for mock MONARCH on its own issue, or on the unit tests one - whatever you prefer.

cguzman95 commented 4 years ago

Well, since mock_monarch multi-cell checking is only for test, when I see a problem with this I will create the issue more to fix this test than anything (I doubt that the user would have a problem with this bug). I also believe this problem should not seen on unit-tests since we are using different classes.

mattldawson commented 4 years ago

ok - it might even be better to move this test to the MONARCH project, rather than having it here. That way we could build a mock_monarch.F90 that uses the real monarch_pmc_interface.F90 (which has changed quite a bit since this test was built)

cguzman95 commented 4 years ago

Yep, having a chemistry_mock_monarch.f90 test on monarch could be grateful. The only problem is the work to implement this (last time I talk to oriol to only make a dummy test configuration to test chemistry it takes a lot of time)