-
Notifications
You must be signed in to change notification settings - Fork 52
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
bug in SAFT-VR SW? #165
Comments
hmm, no idea. SAFT-VR-SW is one of those "legacy" SAFT eos, that are superseeded by more modern approaches (like SAFT-VR-Mie). also, looking at the code, seems that there wasn't any (functional) changes since last year. |
on a preliminary testing,
so, if any bugs present, seems to be a subtle one |
It's been a while since I made that figure but, based on the figure we used in our paper, it seems like we removed SAFT-VR SW from it. I believe that may have been because the results had changed to something like TermeHansen is showing. Ill quickly benchmark SAFT-VR SW to the original paper and see if we can still replicate their results. |
I've now done the benchmark and can confirm that the current implementation of SAFT-VR SW replicates the original papers. There must have been something wrong at the time with the code. |
Good to know, then I suggest to either remove or update the plot here on GitHub, to avoid that misunderstanding again :) Maybe using a plot using plotly which is really nice in giving options to inspect data a bit closer with hovering and zooming. Regards! |
after a long time, i finally found the bug. Clapeyron.jl/src/models/SAFT/SAFTVRSW/SAFTVRSW.jl Lines 214 to 219 in af65700
here, σ[i] should be σ[i,i] . It is fixed on the master branch.
|
Just started to look around in this very nice module - great work!
As a first I have tried to reproduce the Cp of CO2 plot from the frontpage.
using:
I get:
Which seems fine except for the
SAFT-VR SW
. Any pointers to why I see that difference to your plot?The text was updated successfully, but these errors were encountered: