VlachosGroup / openmkm

Opensource software to model heterogeneous catalytic reactions. Based on Cantera
MIT License
14 stars 10 forks source link

Compilation with cantera problematic after lates updates #50

Closed francescalb closed 2 years ago

francescalb commented 2 years ago

Hi,

After the latest commits in August I am no longer able to compile OpenMKM with the Cantera from the forked cantera repo as specified in the openmkm instructions.

git clone https://github.com/mbkumar/cantera.git.
cd cantera
git checkout openmkm

Has the required cantera installation been updated elsewhere? With my openmkm fork from prior to August I have no problems with the installation itself. I am installing with the Dockerfile as in the pull request I made as a suggestion a few months ago.

Best regards, Francesca


This is the end of the errors messages during compilation:
#22 102.7 io.cpp:229:18: error: 'class Cantera::Kinetics' has no member named 'getDeltaRefEntropy'; did you mean 'getDeltaEntropy'?
#22 102.7   229 |             mgr->getDeltaRefEntropy(sRxnRef.data());
#22 102.7       |                  ^~~~~~~~~~~~~~~~~~
#22 102.7       |                  getDeltaEntropy
#22 102.7 io.cpp: In function 'void OpenMKM::print_rxn_gibbs(std::vector<Cantera::Kinetics*>, doublereal, std::string)':
#22 102.7 io.cpp:276:18: error: 'class Cantera::Kinetics' has no member named 'getDeltaRefGibbs'; did you mean 'getDeltaGibbs'?
#22 102.7   276 |             mgr->getDeltaRefGibbs(muRxnRef.data());
#22 102.7       |                  ^~~~~~~~~~~~~~~~
#22 102.7       |                  getDeltaGibbs
#22 113.5 scons: *** [io.o] Error 1
#22 113.5 scons: building terminated because of errors.
mbkumar commented 2 years ago

@skasiraj Could you please take a look into it?

francescalb commented 2 years ago

Hi, so to clarify: VlachosGroup/openmkm commit d74079d works with mbkumar/cantera commit 6c5153d

I cannot find any getDeltaRefEntropy /Gibbs in any cantera version, but getDeltaEntropy/Gibbs is found. The change in openmkm seems to me to have been made where it previously called getDeltaEntropy/Gibbs without the 'Ref'. I assume you have made new functions in a local cantera version that has the 'Ref' and not replaced the original ones in cantera.

mbkumar commented 2 years ago

Hello Francesca,

Shashank and I were working couple of months ago to fix an issue. If I remember correctly, there is a different branch named zeroP. Could you please try that?

Bharat Medasani

Engineer Princeton Plasma Physics Lab (PPPL)

On Tue, Oct 12, 2021 at 10:03 AM francescalb @.***> wrote:

Hi, so to clarify: VlachosGroup/openmkm commit d74079d https://github.com/VlachosGroup/openmkm/commit/d74079db895af26fc285c4a17b8d9448e030ff83 works with mbkumar/cantera commit 6c5153d

I cannot find any getDeltaRefEntropy /Gibbs in any cantera version, but getDeltaEntropy/Gibbs is found. The change in openmkm seems to me to have been made where it previously called getDeltaEntropy/Gibbs without the 'Ref'. I assume you have made new functions in a local cantera version that has the 'Ref' and not replaced the original ones in cantera.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/VlachosGroup/openmkm/issues/50#issuecomment-941043844, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA62VEA5EJHMGXDRMG37E63UGQ54DANCNFSM5FYFOXDQ .

francescalb commented 2 years ago

Hi, thank you for answering. I just checked, and the openmkm and zeroP branches are identical: image

francescalb commented 2 years ago

Aha, now I saw there is a pending pull request from skasiraj/zeroP to your zeroP: https://github.com/mbkumar/cantera/pull/1/files

This probably solves the problem (at least by simply looking at the code). MErging this into your openmkm-branch for cantera then should do the trick (hopefully).

francescalb commented 2 years ago

Update: With skiraj:zeroP it does compile at least, so then only documentation needs to be updated (or branches merged.)

mbkumar commented 2 years ago

I need to merge this. Let me review his code tonight and do the merge.

Bharat Medasani

Engineer Princeton Plasma Physics Lab (PPPL)

On Tue, Oct 12, 2021 at 10:48 AM francescalb @.***> wrote:

Update: With skiraj:zeroP it does compile at least, so then only documentation needs to be updated (or branches merged.)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/VlachosGroup/openmkm/issues/50#issuecomment-941083570, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA62VEFIY45WO2E2BIP54PTUGRDDNANCNFSM5FYFOXDQ .

francescalb commented 2 years ago

Great, thanx! I am very happy to see that openmkm is alive :)

skasiraj commented 2 years ago

Hello @francescalb @mbkumar. Somehow Google Mail decided the notifications from GitHub are spam and did not intimate me and couldn't help you guys sooner...

@francescalb because of the pending pull request I decided to use the changes in my personal "fork" of cantera instead of @mbkumar fork for testing purposes. Unfortunately, this was never updated in the documentation. If you still need assistance please email me at skasiraj@udel.edu and will be happy to provide you updated instructions. Bharat and I will clean this up soon!

Status as of now: a) Use OpenMKM main branch b) Cantera: Use my fork of @mbkumar Cantere. Use the ZeroP branch. https://github.com/skasiraj/cantera/tree/zeroP

However, the latest Docker container at DockerHub: docker pull vlachosgroup/openmkm should have the correct set of libraries, up and running if you want to try in the meantime.

mbkumar commented 2 years ago

Hello Shashank,

I made comments on your pull request. Once you make the required changes, I'll merge them into my cantera fork and merge them into openmkm branch. Lets not update the docs, but the keep the way things are.

Bharat Medasani

Engineer Princeton Plasma Physics Lab (PPPL)

On Thu, Oct 14, 2021 at 10:57 AM Sashank Kasiraju @.***> wrote:

Hello @francescalb https://github.com/francescalb @mbkumar https://github.com/mbkumar. Somehow Google Mail decided the notifications from GitHub are spam and did not intimate me and couldn't help you guys sooner...

@francescalb https://github.com/francescalb because of the pending pull request I decided to use the changes in my personal "fork" of cantera instead of @mbkumar https://github.com/mbkumar fork for testing purposes. Unfortunately, this was never updated in the documentation. If you still need assistance please email me at @.*** and will be happy to provide you updated instructions. Bharat and I will clean this up soon!

Status as of now: a) Use OpenMKM main branch b) Cantera: Use my fork of @mbkumar https://github.com/mbkumar Cantere. Use the ZeroP branch. https://github.com/skasiraj/cantera/tree/zeroP

However, the latest Docker container at DockerHub: https://hub.docker.com/r/vlachosgroup/openmkm should have the correct set of libraries, up and running if you want to try in the meantime.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/VlachosGroup/openmkm/issues/50#issuecomment-943440550, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA62VEHFLWZRPGVEQAI4WDTUG3VXHANCNFSM5FYFOXDQ .

francescalb commented 2 years ago

Hello @francescalb @mbkumar. Somehow Google Mail decided the notifications from GitHub are spam and did not intimate me and couldn't help you guys sooner...

@francescalb because of the pending pull request I decided to use the changes in my personal "fork" of cantera instead of @mbkumar fork for testing purposes. Unfortunately, this was never updated in the documentation. If you still need assistance please email me at skasiraj@udel.edu and will be happy to provide you updated instructions. Bharat and I will clean this up soon!

Status as of now: a) Use OpenMKM main branch b) Cantera: Use my fork of @mbkumar Cantere. Use the ZeroP branch. https://github.com/skasiraj/cantera/tree/zeroP

However, the latest Docker container at DockerHub: docker pull vlachosgroup/openmkm should have the correct set of libraries, up and running if you want to try in the meantime.

Hi, thank you for the info. I was not aware of there being a working docker image available, but now see that it was first added after we started testing out openmkm and the updates are from after I made my merge request. With that, I guess that my merge request is obsolete, and I can close it.

skasiraj commented 2 years ago

I made comments on your pull request. Once you make the required changes, I'll merge them into my cantera fork and merge them into openmkm branch. Lets not update the docs, but the keep the way things are.

Hello Bharat @mbkumar. I made the changes you requested (all but one), pushed my changes and updated the pull request. Please review it.

mbkumar commented 2 years ago

The changes are pushed to mbkumar/cantera:openmkm branch. @francescalb you should be able to compile the code using the openmkm branch. If you confirm it, I'll close the issue.

francescalb commented 2 years ago

thank you, compilation now works!

Francesca

mbkumar commented 2 years ago

Fixed in mbkumar/cantera#1