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

fixes issue #275 #276

Closed
wants to merge 1 commit into from
Closed

fixes issue #275 #276

wants to merge 1 commit into from

Conversation

skyreflectedinmirrors
Copy link
Contributor

No description provided.

@speth
Copy link
Member

speth commented Jun 18, 2015

Nick, thanks for taking a look at this. I'd like to suggest some changes, based on the fact that the underlying reportParameters method is kind of a terrible interface to this data.

The C++ class SpeciesThermoInterpType already has methods for returning the minimum and maximum temperatures and the reference pressure, which could be implemented as properties on the Python class (the names min_temp, max_temp, and reference_pressure would be consistent with the equivalent properties of class ThermoPhase).

The parameterization type is already available as the class attribute derived_type. There might be a more clear name for this, but I'd rather discourage use of the integer identifiers anyway when you can just look at the name of the class.

The coefficients array could be made available as a property coeffs, which would be consistent with the widespread use of properties throughout the Python module. If there's a good reason for it to be a method, the preferred verb should be get.

Finally, the index property is deprecated (I believe it should just return zero for any species), since it's an abstraction violation for the species thermodynamic data to need to know the index of that species within a phase.

@kyleniemeyer
Copy link
Member

Ray: Nick and I were talking about this, and what you describe is actually what I did (not realizing Nick was working on the same thing simultaneously).

I'll submit my pull request after incorporating the tests he added.

@speth speth closed this in cd545af Jun 18, 2015
@speth speth mentioned this pull request Nov 29, 2016
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.

3 participants