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 tp_flash & example notebook #56

Merged
merged 5 commits into from
Dec 4, 2021

Conversation

denbigh
Copy link

@denbigh denbigh commented Dec 2, 2021

This pull request contains a new file, src/methods/property_solvers/tp_flash/DifferentialEvolutiontp_flash.jl, in which a new function, tp_flash, is defined and exported. tp_flash solves the non-reactive flash problem via differential evolution.

I have also included an example notebook, examples/PTFlashDifferentialEvolution.ipynb, to demonstrate use of this function.

I am new to package development in julia, so I apologize if changes need to be made.

Feel free to free to use and modify this code in any way.

@pw0908
Copy link
Member

pw0908 commented Dec 2, 2021

Thinking in terms of tests, what would say is the minimum required to ensure all aspects of your code works?

Would just testing for two-phase equilibrium be sufficient? Or should we test three-phase as well?

@denbigh
Copy link
Author

denbigh commented Dec 2, 2021

I think a two-phase test would be sufficient. There are a number of examples (with 2 and 3 species) in the example notebook. A three-phase simulation should work as well, but I believe you'd need >=4 species to see a 3-phase region of phase space at fixed p and T.

@pw0908
Copy link
Member

pw0908 commented Dec 3, 2021

There's a compatibility issue between BlackBoxOptim.jl and FillArrays.jl which we're trying to resolve. Really, we're just waiting for the BlackBoxOptim.jl folks to update their package...

@pw0908
Copy link
Member

pw0908 commented Dec 4, 2021

I've now switched the optimiser package to Metaheuristic such that we don't have a compatibility issue. I've also added a simple test. Do you think that's sufficient?

@longemen3000
Copy link
Member

If the test passes, merge it, we can iterate over it later

@pw0908
Copy link
Member

pw0908 commented Dec 4, 2021

Do you want to re-organise anything?

@longemen3000
Copy link
Member

Oh, yes, but if the functionality is working, the code reorganization can be done later

@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2021

Codecov Report

Merging #56 (fb57f90) into master (92a613d) will increase coverage by 0.01%.
The diff coverage is 87.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   84.35%   84.37%   +0.01%     
==========================================
  Files         112      113       +1     
  Lines        6877     6917      +40     
==========================================
+ Hits         5801     5836      +35     
- Misses       1076     1081       +5     
Impacted Files Coverage Δ
src/Clapeyron.jl 100.00% <ø> (ø)
..._solvers/tp_flash/DifferentialEvolutiontp_flash.jl 87.17% <87.17%> (ø)
src/solvers/nlsolve.jl 66.66% <0.00%> (+1.96%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92a613d...fb57f90. Read the comment docs.

@longemen3000 longemen3000 mentioned this pull request Dec 4, 2021
@pw0908
Copy link
Member

pw0908 commented Dec 4, 2021

Alright, after 3 runs, I still can't get all tests to pass but I think Andrés' other update should be able to handle it.

@pw0908 pw0908 merged commit bd3c228 into ClapeyronThermo:master Dec 4, 2021
@denbigh denbigh deleted the tp_flash_DE branch December 6, 2021 05:32
@denbigh
Copy link
Author

denbigh commented Dec 6, 2021

Wonderful, thanks for making this process so smooth! I can see you have made my crude code more 'Julian' - I look forward to testing this to see if there are some performance improvements!

pw0908 added a commit that referenced this pull request Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants