Skip to content
This repository has been archived by the owner on Jan 4, 2021. It is now read-only.

Release of new ExternalMedia #13

Open
jowr opened this issue Oct 1, 2015 · 30 comments
Open

Release of new ExternalMedia #13

jowr opened this issue Oct 1, 2015 · 30 comments

Comments

@jowr
Copy link
Member

jowr commented Oct 1, 2015

Francesco would like to release a new version of ExternalMedia soon. Do you @JonWel, @squoilin and @ibell think that we should push for including CoolProp5 in it?

I think the remaining tasks of #4 should be fixed before we include a new CoolProp. If we provide a binary, we cannot expect everyone to help us debugging and the software should work somewhat reliably. How well tested is the current code base? Do we feel comfortable releasing it?

@JonWel
Copy link
Contributor

JonWel commented Oct 1, 2015

  • Tabular method: calc_first_two_phase_deriv_splined is still missing in the tabular backed (required for rho splined). Needs some checking for the behaviour in the two phase region.
  • Testing with current code base: the main problem I experienced was a difference in the enthalpy reference. As the starting enthalpy is quite often specified in the examples, it gives errors when the desired starting point is not on the new domain.
    We should thus first check which reference system where used in CoolProp 4 and are now use by default in CoolProp 5.
    Then, if this is the same reference, maybe checking the bounds in CoolProp 5. If they are different, either change the problematic starting enthalpy points of the examples (and users would have to update their own code in the same way if they encounter the problem), or to impose the old reference system in the ExternalMedia code.

@jowr
Copy link
Member Author

jowr commented Oct 1, 2015

OK, but I guess the more elegant way would be to specify the start values in a way that is independent from the reference state. One could for example use a saturation state an enthalpy difference or something like that.

@ibell
Copy link

ibell commented Oct 1, 2015

Or just start with a T,p pair? That would be the most reliable.

On Thu, Oct 1, 2015 at 6:51 AM, Jorrit Wronski notifications@github.com
wrote:

OK, but I guess the more elegant way would be to specify the start values
in a way that is independent from the reference state. One could for
example use a saturation state an enthalpy difference or something like
that.


Reply to this email directly or view it on GitHub
#13 (comment)
.

@ibell
Copy link

ibell commented Oct 1, 2015

I can see about the phases function. It's a bit of work to make sure that
we are calculating the correct(!) phase everywhere - there are two open
issues on my plate about that. Aside from that, I think the phase
setting/getting should be ok, and I have a rather long amount of time on a
plane/in airports in the coming days, so I might be able to finally close
it out.

On Thu, Oct 1, 2015 at 8:07 AM, Ian Bell ian.h.bell@gmail.com wrote:

Or just start with a T,p pair? That would be the most reliable.

On Thu, Oct 1, 2015 at 6:51 AM, Jorrit Wronski notifications@github.com
wrote:

OK, but I guess the more elegant way would be to specify the start values
in a way that is independent from the reference state. One could for
example use a saturation state an enthalpy difference or something like
that.


Reply to this email directly or view it on GitHub
#13 (comment)
.

@JonWel
Copy link
Contributor

JonWel commented Oct 1, 2015

Test_Cell1D_ht is a Thermocycle example that currently don't run well with CoolProp 5 out of the box.

CO2 TestBasePropertiesExplicit is a simple ExternalMedia example where the enthalpy setting gives an error.

@ibell
Copy link

ibell commented Oct 2, 2015

See my push in CoolProp/CoolProp@80851f4
. I think I fixed it, file issues if not.

On Thu, Oct 1, 2015 at 10:09 AM, JonWel notifications@github.com wrote:

Test_Cell1D_ht
https://github.com/thermocycle/Thermocycle-library/blob/master/ThermoCycle/Examples/TestComponents/Test_Cell1D_ht.mo
is a Thermocycle example that currently don't run well with CoolProp 5
out of the box.


Reply to this email directly or view it on GitHub
#13 (comment)
.

@casella
Copy link

casella commented Jan 20, 2016

Hi,
I am finally back on this, I hope we can get a release of ExternalMedia+CoolPropV5 soon

@casella
Copy link

casella commented Jan 20, 2016

BTW, would it be easier to handle the dependencies if we moved ExternalMedia to Git?

@JonWel
Copy link
Contributor

JonWel commented Jan 20, 2016

Hi,
The main missing feature I'm aware off is the function calc_first_two_phase_deriv_splined in the tabular backend (it is present in the HelmholtzEOS backend and is required for the rho splined option). The ExternalMedia code should be ready for it, but this feature is not yet in CoolProp 5. (Missing feature in term of having at least what previous version could do. Features like mixtures can be added after.)

The example codes haven’t been checked and corrected yet (there is sometimes some issues due to a change of enthalpy reference as I understood).

I updated my checkbox at the beginning of this issue.

For the dependencies, current BuildLib-VS.bat calls git to download and update CoolProp 5. Having the ExternalMedia code on GitHub too would allows a single call git clone --recursive https://pathToExternalMediaGithub.
I think @jowr will have better answer for this question.

@jowr
Copy link
Member Author

jowr commented Jan 20, 2016

Yes, git would simplify things. As for now, I think that we should go for some basic testing (I do not have access to Dymola anymore) and the release a new version with CoolProp 5. I am not sure how many people actually use the splined derivatives, but I guess that we should put a something about it in the release notes.

@JonWel
Copy link
Contributor

JonWel commented Jan 20, 2016

At least one person seems in need for this feature (see issue 16). This function does not only compute the derivatives but is also used to compute the value of the smoothed density : https://github.com/CoolProp/ExternalMedia/blob/master/Projects/Sources/coolpropsolver.cpp#L241 (which may not be obvious by the name).
You can see here that a correct choice of input gives the required smoothed density: https://github.com/CoolProp/CoolProp/blob/master/src/Backends/Helmholtz/HelmholtzEOSMixtureBackend.cpp#L2969

Computing the two derivatives or the two derivatives + rho, in a single function call may be quicker as part of the code is identical. This may be an improvement to do for later if considered worth it (this may be interesting when using the tabular backed as state update is then quite fast).

For the testing, I don't have time until mid February (examination time). At that moment, I'll will have to see with @squoilin what can be done in the frame of my thesis.

@bilderbuchi
Copy link

bilderbuchi commented Jul 12, 2016

Is a new release of ExternalMedia coming?

Also, what is the "canonical"/upstream/definitive repository for ExternalMedia?

  1. This repo?
  2. The one by the Modelica association, apparently maintained by @casella? This is where it seems the CoolProp/ExternalMedia repo was forked from (without using the fork button, so the connection is broken). Also, I think Openmodelica draws its copy from there.
  3. Or, the SVN repo? (I guess not)

If it's 2., I can help with getting the changes from here back upstream, if necessary.

@casella
Copy link

casella commented Jul 12, 2016

The "canonical" repo is the one on the Modelica SVN server. The version with coolprop v5 is currently on a branch. Whatever is shown on GitHub is a mirror, so some functionality might be broken.
I can have a look at this next week and see if we can fix all issues and merge coolprop v5 into the trunk.
we could also move externalmedia to GitHub natively, but this I can only take care of in September.
If anybody out there can help me with that, please let me know!

@JonWel
Copy link
Contributor

JonWel commented Jul 12, 2016

To my understanding the main missing element is explained in #20 and #16 , and lies on the CoolProp's tabular backend side (ie once implemented in CoolProp it should work out of the box with this ExternalMedia repository)

Currently the compilation has mainly be tested under Visual Studio. The compilation under Linux lack testing (#21) and I have no idea if the OMC script still work.

@bilderbuchi
Copy link

we could also move externalmedia to GitHub natively, but this I can only take care of in September.
If anybody out there can help me with that, please let me know!

sure thing, what do you need specifically?
As the modelica/ExternalMedia repo on Github is already a mirror, you only need someone to pull the coolprop v5 branch to github, correct? Also, probably open the issue tracker there.

@bilderbuchi
Copy link

@casella also, in which way do you want to reconcile the work by @JonWel and @jowr in the Coolprop/ExternalMedia repo with the newly canonical github one? Is the work compatible?

@bilderbuchi
Copy link

So, I just completed a Github import of the SVN repo, it can be found here.

Interestingly, the last commit in the coolprop5 branch by @jowr says "Synchronising with 6f50f6d ". Since then there have been more commits in CoolProp/ExternalMedia, so the most recent state is actually in CoolProp/ExternalMedia, and there is no need to "move to github natively", as there is no new state we could transfer (the other branches have latest commits from 2013, so I don't think there is usable code in there, right @casella ?)

The only thing I could imagine being useful is bugging Github support about connecting CoolProp/ExternalMedia with the upstream repo modelica/ExternalMedia - apparently CoolProp/ExternalMedia was not created with the fork button, so this connection is broken (no "forked from blabla text available), and it's much more difficult to send PRs from one to the other.
I can do that if @casella @jowr agree that this is the way to go. Then, we can easily send changes upstream via PRs, and new releases of modelica/ExternalMedia should get picked up by other Modelica tools (e.g. Openmodelica).

@jowr
Copy link
Member Author

jowr commented Jul 15, 2016

Thank you for the suggestions. My idea was to have a sandbox repository here for us to play with the code. Whenever we get something working, I would sync this repo to the SVN server. This is because this repository breaks every now and then and the code on the SVN should at least be compilable...

I have no problem with changing this, but I would like to stress that the repositories work as they should and I am happy to merge things from here to the SVN on demand.

@bilderbuchi
Copy link

I see, so modelica/ExternalMedia would really serve as your upstream.

Re: Github vs SVN I am extrapolating (from my own experience/feeling) that it would be much easier for other people to contribute or inspect the current state of development, if both repos were on Github.
Also, getting changes from here to upstream would remain easy via the PR mechanism, without the history mangling of manual code synchronizations like in the coolprop5 branch I linked above.

I guess it boils down to what @casella wants: If you want to switch the main upstream repo to Github, just say the word and I'll bug support about restoring the link between the two, so that PRs can be easily created.

@jowr
Copy link
Member Author

jowr commented Jul 15, 2016

Fine with me. Please let me know if I should merge the current state into the SVN prior to the transition to GitHub - if @casella wants to move to Git as the primary VCS.

@bilderbuchi
Copy link

bilderbuchi commented Jul 15, 2016

I don't think we should copy the SVN repo to Github, to be honest. Right now, both the Coolprop and the Modelica one have one ancestry (i.e. same commit hashes, compare this and that), so it's easy to share/transfer code between.
However, the repo as imported from SVN has different hashes (this could be due to not completely associated usernames, or something else), so it's not nicely mergeable. All the other branches in the SVN are old, so my feeling is that they can be discarded/not mirgrated (@casella could confirm if this is ok), the contents of coolprop5 can be sent upstream from the coolprop repo (where it is the current master) when ready, and we're done and current.

@jowr
Copy link
Member Author

jowr commented Dec 1, 2016

I have fixed a couple of bugs and if someone could help me testing, we might actually get close to a release.

jowr added a commit that referenced this issue Dec 1, 2016
@casella
Copy link

casella commented Dec 2, 2016

Hi Jorrit,
I can try to help next week. Can we have a Skype talk on Friday 3 to coordinate?

@bilderbuchi
Copy link

bilderbuchi commented Oct 6, 2017

@casella would you please give an update here of the new, current situation?
What is the plan regarding this CoolProp/ExternalMedia repo?
Delete it and work on solely on https://github.com/casella/ExternalMedia as the canonical upstream?
Is there anything actionable from the above discussion that you would like to have done?

@casella
Copy link

casella commented Oct 6, 2017

@bilderbuchi, I wrote an email to several people including yourself yesterday evening. Did you get it?
I wrote a summary here https://github.com/casella/ExternalMedia/wiki, if it is not clear please open an issue there.
My plan is to wait deleting anything until we have a release on https://github.com/casella/ExternalMedia, then link it to the Modelica GitHub repo and delete all other instances only then. Just in case.
As to CoolProp, the plan is to stop including the source code into ExternalMedia as we did in the past, which is really ugly and difficult to manage, but rather link to it via a DLL. This requires some changes to the core code of the library that I will work on in the next few days. See the wiki

@bilderbuchi
Copy link

Yes I got the email, which prompted me - not everyone stumbling over this issue got it, probably. :-)

Yeah, I was referring to the repo under the coolprop organisation, i.e. the one in whose issue tracker we are right now. Sure, it's smart to wait with deleting, it should just be clear what the canonical repo is w.r.t to source, issue tracker, ... :-)

@casella
Copy link

casella commented Oct 6, 2017

I guess it would be wise to delete this repo, but only when we demonstrate on the new canonical one that everything works fine (or in fact, better than it does now).

@jowr
Copy link
Member Author

jowr commented Oct 9, 2017

Agreed, the only thing is that we made this repo work with CMake on Linux and Windows. Would you be interested in this functionality? I might be able to make a slimmed down commit to your repo that ships the CMake stuff over.

@bilderbuchi
Copy link

FYI, this is the diff from the modelica repo master to the coolprop repo master, excluding whitespace changes.

@JonWel
Copy link
Contributor

JonWel commented Oct 10, 2017

@jowr There was also the changes in Projects/Sources/coolpropsolver.cpp to make it use CoolProp 5 (then CoolProp 6) instead of CoolProp 4.

But I agree with the idea that it's better to have two distinct entities with a more stable interface in between, rather than integrating CoolProp inside ExternalMedia as before, and having the link breaking frequently with CoolProp's updates.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants