KineticPreProcessor / KPP

The KPP kinetic preprocessor is a software tool that assists the computer simulation of chemical kinetic systems
GNU General Public License v3.0
21 stars 11 forks source link

Matlab #45

Closed RolfSander closed 2 years ago

RolfSander commented 2 years ago

This is for all matlab-related issues

RolfSander commented 2 years ago

I have created a new branch called matlab in which I have introduced some preliminary fixes:

https://github.com/KineticPreProcessor/KPP/tree/matlab

@ShaddyAhmed: Does this work for you?

@yantosca, @msl3v: There were two problems. First, Matlab does not have the functionFile and I had to deactivate some write statements related to Aout and Vdotout for Matlab. Second, Matlab does not use WriteOMPThreadPrivate so I had to deactivate those calls as well. Can you please check if you agree with my changes?

yantosca commented 2 years ago

Thanks @RolfSander. I am out sick but will take a look by early next week. I think it should be fine to shunt those write statements for Matlab (as they are only needed for F90).

RolfSander commented 2 years ago

Get well soon!

msl3v commented 2 years ago

I'll have a look this week. ML

ShaddyAhmed commented 2 years ago

@RolfSander Yes I can now compile the mechanism successfully! I still need to perform some tests to check that the mechanism works (there are some warnings regarding continuation lines in the rate laws that will need to be fixed). I will report any bugs/issues on this thread.

yantosca commented 2 years ago

Hi all, sorry for the late response, I was out sick since coming back from the IGC10 meeting last week.

@RolfSander, I took a look at the matlab branch and ran the C-I tests on that vs. the dev branch. We get identical results.. the only diffs are the ros_split test and the seulex90 test which are in one branch but not the other. So unless there are any other issues in generating the Matlab code, I think we are good to go.

yantosca commented 2 years ago

Also tagging Katie Travis (@ktravis213) at EPA. Katie uses the FOAM box modeling framework, which runs in Matlab. Would be great if we can test a KPP-generated Matlab mechanism w/ FOAM at some point.

RolfSander commented 2 years ago

@jenniethomas, @ShaddyAhmed, @ktravis213: We are making good progress with our KPP development and hope to release version 3.0.0 soon. It would be good to discuss how we treat the Matlab output of KPP. We've been told that a couple of fixes must be applied before the KPP-generated Matlab code can be used:

If these are generic problems (not specific to PACT-1D or FOAM), we could try to fix them directly in the KPP source code. I see two options:

1) We include this quite soon in KPP-3.0.0.

2) We include this later, and will release it as, e.g., KPP-3.0.1.

Which option do you prefer? I think it would be nice to have this fixed in 3.0.0 already. However, it depends of course on how much time you're able to invest into this now.

jenniethomas commented 2 years ago

Also posting here - @ShaddyAhmed @JPSTUTZ and @ThorBarRa are working with Matlab mechanics at the moment and would be happy to test the new version.

ShaddyAhmed commented 2 years ago

Hi all - sorry for the delay in getting back to you on this! I did some tests compiling the PACT-1D mechanism with KPP 2.5.0 and there are just a few minor bugs which I think can be fixed in the KPP source code:

  1. Update_RCONST: Line continuation error for one rate constant (RCONST(46)).
  2. Fun: 2 fixes - Vdotout variable is not defined and allocate arrays for A and Vdot.
  3. Jac_SP: Allocate arrays for B and JVS.

I also checked the monitor file and this seems to be working fine from KPP with no need for any changes. The KPP-generated files, fixed files and KPP output are all attached.

KPP output: KPP_output.txt

KPP generated files: KPP_mech_Fun.txt KPP_mech_Jac_SP.txt KPP_mech_Update_RCONST.txt

Fixed files: FIXED_mech_Fun.txt FIXED_mech_Jac_SP.txt FIXED_mech_Update_RCONST.txt

RolfSander commented 2 years ago

Thanks, @ShaddyAhmed !

I think I will be able to transfer these fixes into the KPP source code.

I also noticed that your fix added global ClAer_t0 and global BrAer_t0 to Update_RCONST. Wouldn't it be better to add these lines via #INLINE MATLAB_RCONST?

ShaddyAhmed commented 2 years ago

@RolfSander Great - Sorry I forgot to remove those lines as they were simulation specific for our case and not needed for other cases. These lines can be ignored

jenniethomas commented 2 years ago

@RolfSander these were rather specific hacks for our model. They should not be added. :)

jenniethomas commented 2 years ago

@RolfSander Great - Sorry I forgot to remove those lines as they were simulation specific for our case and not needed for other cases. These lines can be ignored

sorry - we responded at the same time. We agree on this. So, don't add.

RolfSander commented 2 years ago

@yantosca: While looking at Vdotout and Vdot in Matlab, I noticed that we have this also in the f90 output. Is the new optional parameter Vdotout in SUBROUTINE FUN really necessary? The same values are already returned in the variable Vdot.

RolfSander commented 2 years ago

I have written some bug fixes and pushed them into the matlab branch. @ShaddyAhmed: Can you test if it works for you?

The line continuation error is still not fixed properly. As a Q&D bug fix, I simply allow longer lines now. If this is not good enough, we'll have to look at this problem again.

ShaddyAhmed commented 2 years ago

@RolfSander Thanks! I just tested the updates and they are working fine for me!

RolfSander commented 2 years ago

Great!

I've merged the Matlab updates updates into the dev branch, so they will be available when KPP-3.0.0 is released.

Is there anything else that needs to be done for Matlab? If not, I will close this issue.

ShaddyAhmed commented 2 years ago

Sounds great! No other updates from me for now. @jenniethomas anything to add?

jenniethomas commented 2 years ago

No more from me! I think we will just have Jochen and Thorsten test with their running versions. I will ask them now and you should have the answer shortly.

RolfSander commented 2 years ago

Thanks, @jenniethomas and @ShaddyAhmed! I will close this issue now.

If you find any additional bugs, feel free to re-open this issue any time (or create a new issue).

jenniethomas commented 2 years ago

@RolfSander Thorsten is just checking one things works for him - we will confirm Monday. We are just checking he used the right branch and will follow up.

RolfSander commented 2 years ago

No problem. Just re-open this issue if there is anything else to do...

You can use the dev branch now because the matlab branch has been merged into dev.

ThorBarRa commented 2 years ago

@RolfSander These issues are fixed and KPP compiled and runs successfully. This is in a Apple M1 Pro with MacOS Monterey (12.4). Thanks for your work on the code to make compiling it on Mac work! There were additinonal workaround needed to compile it on 12.4 related to native gcc not running well (so one has to use homebrew version instead) and to the location of the library which Apple seems to shift around with every update. Happy to share those.

jimmielin commented 2 years ago

Thanks @ThorBarRa! Please do let us know the changes you made for KPP to work on macOS 12.4. Perhaps we can include the macOS-related information in the Installation section of the online documentation (https://kpp.readthedocs.io/en/latest/getting_started/01_installation.html). I think we briefly mention homebrew, but we do not specify the gcc version that works with KPP on macOS or other fixes needed. I believe @yantosca also has experience running KPP on macOS and we could put together a list of 'hacks' needed in the documentation for future reference.

yantosca commented 2 years ago

Thanks @ThorBarRa and @jimmielin. I can add the info for MacOS to the doc later this week. I believe I was using GCC 11.4.

Recent Mac-specific commits were: 4d919d0c7ed0c692a05a616ddc72a191a0bbd0cd, e01a5449b2408f33681892deff850064b7a395b9, 6da889145caa8d21e8f73abd694d23bb77398abd

jenniethomas commented 2 years ago

Thanks all for this excellent progress, let's hope this is really useful for users within the community. @yantosca and @RolfSander thanks for contacting us to mobilize to work on this.

JPSTUTZ commented 2 years ago

I successfully tested the KPP 2.6 version with two of our current models. I did not encounter any issues.

JPSTUTZ commented 2 years ago

One additional question: Is there a way to eliminate the use of global variables in KPP for Matlab.

Matlab is quite inefficient in its use of global variables, i.e. it makes the code slow. In addition, the use of global variable prevents parallelization of our model.

RolfSander commented 2 years ago

Thanks for the positive feedback, @JPSTUTZ!

I'm not using Matlab, so I don't know if it is possible to eliminate global variables. However, if you find a way to do this, I should be able to adjust the KPP-generated Matlab code accordingly.

jimmielin commented 2 years ago

I'm not familiar enough with Matlab's issue on global variables but I was wondering if we can tie this with the move to derived types (equivalent to struct in C or a structure array in Matlab) which we wanted to do in Fortran 90 for KPP 4.0.0 - https://github.com/KineticPreProcessor/KPP/issues/11

It seems like moving KPP global variables to a structure array in Matlab could help with parallelization, and maybe make the slowness a bit better. I would love to hear your thoughts on how to move away with global variables, @JPSTUTZ!

JPSTUTZ commented 2 years ago

From what I can find on the performance of structures in Matlab is that they would be faster than global variables. They are a little slower than just using arrays, but I think the benefit of cleaner code outweighs that. Structures in matlab work pretty much like in C, so I think that would be the way to go.

yantosca commented 2 years ago

@ThorBarRa, I added some documentation for MacOS to ReadTheDocs. It should be visible at https://kpp.readthedocs.io/en/latest/.

ThorBarRa commented 2 years ago

@yantosca Thanks! I'll try it over the weekend and come back to you? Remind me. Was that for silicone Macs and macOS 12? And, apologies for my silence....trying to understand and modify PACT-1D :-)

ThorBarRa commented 2 years ago

" From what I can find on the performance of structures in Matlab is that they would be faster than global variables. They are a little slower than just using arrays, but I think the benefit of cleaner code outweighs that. Structures in matlab work pretty much like in C, so I think that would be the way to go."

I can just copy that. Structure would slimmer and one could rely more on newer built-in features and functions that Mathworks introduced for structures (and similar data containers). Happy to contribute, once its time for 4.0.

RolfSander commented 2 years ago

@jenniethomas, @ThorBarRa, @JPSTUTZ, @ShaddyAhmed: We have removed Vdotout from KPP because it turned out that it was redundant. We also removed the line

Vdotout = Vdot(:);

from the Matlab output. We don't think it is needed but please let us know if it is...

ShaddyAhmed commented 2 years ago

@RolfSander Yes I imagine this should work fine for matlab!

RolfSander commented 2 years ago

OK, thanks @ShaddyAhmed!