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

Add HIC Binding Models #121

Merged
merged 4 commits into from
Apr 28, 2023
Merged

Add HIC Binding Models #121

merged 4 commits into from
Apr 28, 2023

Conversation

schmoelder
Copy link
Contributor

@schmoelder schmoelder commented Dec 17, 2022

This PR combines #116 and #118 .

To do

  • Finalize model naming
  • Add Documentation (@ronald-jaepel, could you please check the docs and provide us with the units of the model parameters.)
  • Add Tests (unfortunately, i nuked the test file while it was still in progress. 😠 Need to revisit in order to check parameters. )

@ronald-jaepel provided us with some typical test parameters for HICWHS:

  • BETA0: 0.0184390384521496
  • BETA1: 0.000797098469630127
  • KA_c1: 0.872767843365959
  • KD_c1: 44.9707701873943
  • KEQ_c1: 0.0194074470979508
  • NU_c1: 10
  • QMAX_c1: 1000

And for HICCWA:

  • BETA0: 0.326934960685602
  • BETA1: 0.000221461823367185
  • KA_c1: 0.474361388031419
  • KD_c1: 2045.88163309217
  • KEQ_c1: 0.000231861599595311
  • NU_c1: 10
  • QMAX_c1: 10

@lieres could you have a look at the naming of the isotherms? We decided not to call them "Wang" and "Jäpel" isotherms, instead they are now called "HIC - Water on Hydrophobic Surfaces" and "HIC - Constant Water Activity".

@schmoelder schmoelder changed the title Feature/add hic binding Add HIC Binding Models Dec 17, 2022
@schmoelder schmoelder force-pushed the feature/add_hic_binding branch 2 times, most recently from 94c2622 to 6706fc8 Compare December 17, 2022 11:46
@ronald-jaepel
Copy link
Collaborator

Thanks for your help!
I can't figure out how to properly push to this Pull Request, so here are the doc files for the interface section:
docs.zip

@schmoelder
Copy link
Contributor Author

Thanks a lot! I can update it myself.

In case you're interested in how to push to this repo, you'll have to add our Github repository as remote repository.
With git, you can have multiple remote repositories. The default remote is usually called origin. But origin is just a tag and you have others. For example, you could have a personal fork on Github that you might want to call github.

To add,

git  remote add github https://github.com/ronald-jaepel/CADET.git

and ours could be origin or upstream

git remote add [origin/upstream] https://github.com/modsim/CADET.git

To check existing remote locations, use

git remote -v

Finally, when you push, you can now specify the location.

git push [origin/upstream] feature/add_hic_binding

@schmoelder
Copy link
Contributor Author

schmoelder commented Jan 20, 2023

Hey, I added some tests to both HIC models using the parameters you provided and adding other components. Unfortunately, there still seem to be some issues. Here is a part of the output:

/home/jo/software/CADET/test/BindingModelTests.cpp:247: FAILED:
  CHECK( jacAna.native(row + cbm.nComp(), col) == makeApprox(jacAD.native(row, col), absTol, relTol) )
with expansion:
  0.474361388 == RelApprox( 0.0 )
with messages:
  row := 0
  col := 5

/home/jo/software/CADET/test/BindingModelTests.cpp:247: FAILED:
  CHECK( jacAna.native(row + cbm.nComp(), col) == makeApprox(jacAD.native(row, col), absTol, relTol) )
with expansion:
  5.6923366564 == RelApprox( 0.0 )
with messages:
  row := 1
  col := 4

/home/jo/software/CADET/test/BindingModelTests.cpp:247: FAILED:
  CHECK( jacAna.native(row + cbm.nComp(), col) == makeApprox(jacAD.native(row, col), absTol, relTol) )
with expansion:
  2.847348005 == RelApprox( 0.0011796768 )
with messages:
  row := 1
  col := 5

double free or corruption (!prev)
malloc(): corrupted top size
Abgebrochen (Speicherabzug geschrieben)

I will double check whether I made some mistake while setting up the test or if this is really an issue with the implementation (of the jacobian). In any way, I wanted to push what I've got so far.

@schmoelder
Copy link
Contributor Author

The pipeline failed with:

 Err:3 http://azure.archive.ubuntu.com/ubuntu jammy-updates/main amd64 libcurl4-openssl-dev amd64 7.81.0-1ubuntu1.8
  404  Not Found [IP: 40.119.46.219 80]
Fetched 5943 kB in 2s (3781 kB/s)
E: Failed to fetch http://azure.archive.ubuntu.com/ubuntu/pool/main/c/curl/libcurl4-openssl-dev_7.81.0-1ubuntu1.8_amd64.deb  404  Not Found [IP: 40.119.46.219 80]

I assume, to fix, we simply need to add sudo apt-get update to the pipeline (see actions/runner-images#5470).

The dispatch pipeline always fails. It should probably be revised at some point.

@schmoelder
Copy link
Contributor Author

You have a syntax error in your yaml file. See: https://github.com/modsim/CADET/actions/runs/4510620259/workflow

@schmoelder
Copy link
Contributor Author

@ronald-jaepel ready to merge?

@ronald-jaepel
Copy link
Collaborator

@schmoelder I think yes

@schmoelder
Copy link
Contributor Author

schmoelder commented Mar 24, 2023

Cool! Congrats!
I would suggest cleaning up the commits a bit. Do you know how squashing and rebasing works?
@sleweke do you also want to review?

@ronald-jaepel
Copy link
Collaborator

Not yet but I'll read into it.

@ronald-jaepel
Copy link
Collaborator

Read into it. Should all changes be squashed into a single commit or one per Isotherm or any other suggestions?

@schmoelder
Copy link
Contributor Author

haha, you're a machine! :)

Btw, I can highly recommend checking out Lazygit (kudos to @jayghoshter for introducing it to me!).
It makes these things so much nicer.

Should all changes be squashed into a single commit or one per Isotherm or any other suggestions?

Since they are very related, I think it's fine to put them all into a single commit. But I'd make sure to leave the CI fix on a separate commit.

Note that rebasing "rewrites" history. So it's very powerful but can also be dangerous. You will also need to force push. And that's something we should never do on master. But on feature branches it's fine (and sometimes necessary, if your upstream branch has advanced).

Adds the possibility to assign a default value to a parameter in the
template used to generate the actual binding model and reaction model
code. If the underlying parameter class supports it, the provided
default value is assigned to the parameter if it is not found in the
ParameterProvider.
Fixes the literature reference in the code and the docs of the HIC
binding models. Also removes unnecessary casts and promotes the constant
water activity to a (constant) parameter with a default value.
@schmoelder schmoelder merged commit 4b55de8 into master Apr 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants