add params to solid solution about burnman HOT 9 CLOSED

Are you talking about

def __init__(self, endmembers, solution_model=SolutionModel(), molar_fractions=None):

? That looks fine to me.

from burnman.

It's the solution_model part that bugs me. For example, the asymmetric solid solution init looks like this:

def __init__( self, endmembers, enthalpy_interaction, volume_interaction = None, entropy_interaction = None ): 

Rather than

def __init__(self, endmembers, solution_model=SolutionModel(), molar_fractions=None):

I would much prefer something that looks like the mineral init, i.e.

def __init__( self, molar_fractions=None ) 

with

        if 'params' not in self.__dict__:
            self.params = {}
        self.method = None
        if 'model_type' in self.params:
            self.set_solution_model(self.params['model_type'])

I think this would be much more consistent with what we have for the Mineral class, not to mention easier to read, edit and reinitialise (for example, during fitting of solid solution parameters)

from burnman.

Hrm. I am not convinced that the dictionary params instead of real parameters makes things easier here (or that it was a great idea for Mineral for that matter). Real parameters can have nice defaults and can be addressed by names. I don't think params is a very pythonic way of doing things.

What parameters of SolidSolution would you stick into a params dictionary?

from burnman.

All of them (endmembers, enthalpy, entropy interactions ...).
Although you're right, maybe attributes would be better. So you would favour something like the following?

class solid_solution(burnman.SolidSolution):
        def __init__(self, molar_fractions=None):
            self.name='Garnet'
            self.model_type='symmetric'
            self.endmembers = [[minerals.HP_2011_ds62.py(), '[Mg]3[Al]2Si3O12'], [minerals.HP_2011_ds62.alm(), '[Fe]3[Al]2Si3O12']]
            self.enthalpy_interaction =[[0.]]
            burnman.SolidSolution.__init__(self, molar_fractions)

This would also be good for mineral, although maybe it's already too late for such a rewrite.

from burnman.

Actually, thinking about it a bit more, I think params are the best way to pass parameters from SolidSolution to a SolutionModel class. Anything else starts looking very clunky. The params would only be used on init anyway.

I've coded up one possibility. The front end looks like this:

    class solid_solution(burnman.SolidSolution):
        def __init__(self, molar_fractions=None):

            self.params={'name':'Garnet',
                         'type':'symmetric',
                         'endmembers': [[minerals.HP_2011_ds62.py(), '[Mg]3[Al]2Si3O12'],[minerals.HP_2011_ds62.alm(), '[Fe]3[Al]2Si3O12']],
                         'enthalpy_interaction':[[0.]]}

            burnman.SolidSolution.__init__(self, molar_fractions)

To my eyes, this looks cleaner and less confusing than what we had before, removing the need for the user to think about/change the format of init for each solid solution. Additionally, the params don't disappear after init, which has really been annoying me recently. The same data was pushed to burnman.solution_model, but under a different name and in a different format, which I don't think the user should be expected to know.

from burnman.

This looks nice, but is it better than

       burnman.SolidSolution.__init__(self, name = 'Garnet', type= 'symmetric', endmembers=[...], enthalpy_interaction = ..., molar_fractions)

or

self.name = 'Garnet'
self.type = 'symmetric'
self.endmembers = [[minerals.HP_2011_ds62.py(), '[Mg]3[Al]2Si3O12'],[minerals.HP_2011_ds62.alm(), '[Fe]3[Al]2Si3O12']],
                         'enthalpy_interaction':[[0.]]
burnman.SolidSolution.__init__(self, molar_fractions)

?

from burnman.

Your first suggestion ok, but the number of variables is dependent on the "type", and sometimes we want entropy_interaction or volume_interaction to be absent. Also, I don't think it's very easy to read.

I think your second suggestion is best ... it saves a few lines in solidsolution.py, and self.type is much easier on the eye than self.params['type']. I like attributes.

What do you think? Is this a good thing to make a PR for now? It is a reasonably big change, but I don't think there are many people making their own solid solutions with burnman yet...!

from burnman.

Your first suggestion ok, but the number of variables is dependent on the "type", and sometimes we want entropy_interaction or volume_interaction to be absent.

Which is fine, because you can skip arguments if they have default values.

Also, I don't think it's very easy to read.

Fair enough.

I think your second suggestion is best ... it saves a few lines in solidsolution.py, and self.type is much easier on the eye than self.params['type']. I like attributes.

Yes.

What do you think? Is this a good thing to make a PR for now? It is a reasonably big change, but I don't think there are many people making their own solid solutions with burnman yet...!

Go ahead, no problem in changing this.

from burnman.

Excellent, PR #126 now opened.

from burnman.