ComputationalThermodynamics / MAGEMin_C.jl

Julia interface to the MAGEMin C package
GNU General Public License v3.0
12 stars 4 forks source link

Allow @view array for compositions on single point #21

Closed Iddingsite closed 8 months ago

Iddingsite commented 8 months ago

Hi,

When working on my projects, I often create arrays containing vectors of composition. In this case, I like to use @view array to prevent allocations when I am calling a single point minimisation. Currently, this is not supported by single_point_minimization because it only accepts Vector{Float64}.

This PR adds support for AbstractVector{Float64} to be able to use view arrays but also other type of arrays (I didn't go too wild on the multiple dispatch because I don't know how the C code works behind the hood).

So now, this kind of code is working:

    data    = Initialize_MAGEMin("ig", verbose=false);

    # One bulk rock for all points
    P,T     = 10.0, 1100.0
    Xoxides = ["SiO2"; "Al2O3"; "CaO"; "MgO"; "FeO"; "Fe2O3"; "K2O"; "Na2O"; "TiO2"; "Cr2O3"; "H2O"];
    X       = [48.43; 15.19; 11.57; 10.13; 6.65; 1.64; 0.59; 1.87; 0.68; 0.0; 3.0];
    sys_in  = "wt"

    X = [[1.0], X]
    X_view = @view X[2,:]

    out     = single_point_minimization(P, T, data, X_view, Xoxides=Xoxides, sys_in=sys_in)

The only drawback is that it is a breaking PR because I had to change the argument X to be from a keyword argument to a position argument to use multiple dispatch.

So the function single_point_minimization is now called:

single_point_minimization(P, T, data, X, Xoxides=Xoxides, sys_in=sys_in)

Compared to

single_point_minimization(P, T, data, X=X, Xoxides=Xoxides, sys_in=sys_in)

before. EDIT Same for multi_point_minimization.

I've also added a test for testing view.

Tell me what you think about it.

Iddingsite commented 8 months ago

Test failure seems unrelated to the PR.

boriskaus commented 8 months ago

Looks good to me - thanks! you would have to update the readme as well in that case. And it would be good to sort out why the test failed

Iddingsite commented 8 months ago

Errors in testing in Ubuntu randomly occur on all versions due to Segfault errors. I guess it is related to #17? Otherwise, seems good to merge to me!

boriskaus commented 8 months ago

a looked a bit more carefully at this and you are right that the segmentation faults occur randomly. It is curious, though, that they all seem to appear in the test with @view that is being added with this PR.

Iddingsite commented 8 months ago

mmh ok that's strange. I guess there is a weird interaction with the view and MAGEMin with multithread on Ubuntu. I guess I could delete the test. The implementation in itself is just multiple dispatch, so it doesn't change anything if the user uses classical arrays.

boriskaus commented 8 months ago

having the test in itself is good. Nico just released a new version of MAGEMin, which also solves several memory leaks. It may resolve these issues.

Iddingsite commented 8 months ago

This PR is already in the new version of Nico so I will close this one. We will see if the tests pass or not. Sorry for the noise