Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matlab #45

Closed
RolfSander opened this issue Jun 17, 2022 · 38 comments
Closed

Matlab #45

RolfSander opened this issue Jun 17, 2022 · 38 comments
Labels
target-languages Related to the language options for KPP-generated code
Milestone

Comments

@RolfSander
Copy link
Contributor

This is for all matlab-related issues

@RolfSander
Copy link
Contributor Author

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?

Copy link
Contributor

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
Copy link
Contributor Author

Get well soon!

@msl3v
Copy link
Contributor

msl3v commented Jun 19, 2022 via email

@ShaddyAhmed
Copy link
Collaborator

@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
Copy link
Contributor

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
Copy link
Contributor

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.

@yantosca yantosca added the target-languages Related to the language options for KPP-generated code label Jun 21, 2022
@RolfSander RolfSander added this to the 3.0.0 milestone Jun 23, 2022
@RolfSander
Copy link
Contributor Author

@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:

  • fix monitor file
  • add lines to allocate arrays in Fun
  • fix Update_RCONST

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

@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
Copy link
Collaborator

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

@jenniethomas
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@RolfSander
Copy link
Contributor Author

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
Copy link
Collaborator

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

@jenniethomas
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

@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
Copy link
Contributor Author

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
Copy link

@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
Copy link
Member

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
Copy link
Contributor

yantosca commented Jul 5, 2022

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: 4d919d0, e01a544, 6da8891

@jenniethomas
Copy link
Collaborator

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
Copy link

JPSTUTZ commented Jul 6, 2022

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

@JPSTUTZ
Copy link

JPSTUTZ commented Jul 6, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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 - #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
Copy link

JPSTUTZ commented Jul 6, 2022

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
Copy link
Contributor

yantosca commented Jul 6, 2022

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

@ThorBarRa
Copy link

@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
Copy link

" 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
Copy link
Contributor Author

@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
Copy link
Collaborator

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

@RolfSander
Copy link
Contributor Author

OK, thanks @ShaddyAhmed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
target-languages Related to the language options for KPP-generated code
Projects
None yet
Development

No branches or pull requests

8 participants