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

FIX: parameters with components not in phase constituents producing incorrect models #258

Merged
merged 19 commits into from Dec 6, 2020

Conversation

igorjrd
Copy link
Contributor

@igorjrd igorjrd commented Dec 22, 2019

Some improvements in filtering parameters methods, mainly Model._purity_test. New tests related to tests _array_validity and _interaction_test. Fixes #256.

pycalphad/model.py Outdated Show resolved Hide resolved
pycalphad/model.py Show resolved Hide resolved
dbf = Database(TDB_PARAMETER_FILTERS_TEST)
interacting_consts = dbf.search(
where('parameter'))[4]['constituent_array']
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the order of the parameters returned by the search method guaranteed to be in the same order by TinyDB? If not, this test could be a source of flaky, random failures. (This goes for all tests here that assume an order of the parameter list)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default tinydb storage implementation uses dicts, so I would expect order to be preserved as long as the database isn't modified, but this detail should not be relied upon.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better way to write this test would be to just check the result (that you get an error when you expect). That way we are free to adjust internal Model APIs without needing to modify tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bocklund I have tried many tests and the order of parameters seems to not change. Anyway, I was rewriting the search conditions with the addition of tests to get only the parameters with specified sublattices. With this change, the TDB could be modified with minor chance to influence in tests.
@richardotis until now, the way that I see to make errors be raised due _array_validity, _purity_test or _interaction_test is testing their with phases or comps conditions that would build a phase with no parameter (I am not sure if actually this raises an error, but I really expect that it raises). Do you think that this would be a nice design to it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igorjrd My preference would be just to calculate the energy at one temperature and composition. If the database returns the wrong parameters, you get the wrong energy. You just have to first make sure that the test fails without your fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I submitted a new commit that tests the functions using parameters selected by sublattices comps, but I can do some effort to write tests according your suggestion, @richardotis. (I just sent this, because the work was almost done, but it can be reverted).

@bocklund
Copy link
Collaborator

Thanks @igorjrd, I think this is close and the changes and improvements are welcome. When you make your next changes, will you please rebase or merge in the develop branch so the tests will pass on Travis and Appveyor across Python 3.6-3.8, which we are now supporting?

@bocklund
Copy link
Collaborator

Hey @igorjrd, sorry it's been awhile since I've looked at this. I think this is almost ready.

The tests are improved now that they don't depend on the order of parameters in the database, but I agree with Richard that the tests could be improved:

@richardotis: My preference would be just to calculate the energy at one temperature and composition. If the database returns the wrong parameters, you get the wrong energy. You just have to first make sure that the test fails without your fix.

I think adding some tests for the energy to your tests would be enough to merge what you have. See the check_energy function and examples from the test_energy.py module for how to do this.

@bocklund
Copy link
Collaborator

@igorjrd are you still interested in working on this PR?

@igorjrd
Copy link
Contributor Author

igorjrd commented Mar 18, 2020

@igorjrd are you still interested in working on this PR?

Sorry about this long time with no updates. I'm still interested in working on this PR! These last times I could not work properly on this PR, due my masters research activities.
However, in Brazil we are starting to adopt quarentine due covid-19 crysis. I think that until next week I would resume the work on this PR.

@bocklund
Copy link
Collaborator

Great, no rush! Your research should definitely be your priority.

Stay safe and let us know if you have any questions when you are able to pick this up again.

@@ -463,13 +451,18 @@ def _array_validity(self, constituent_array):
# species can be specified in the anion sublattice without any
# species in the cation sublattice.
if self._dbe.phases[self.phase_name].model_hints.get('ionic_liquid_2SL', False):
pass
for sublattice in constituent_array:
if (set(sublattice).issubset(self.constituents[1]) \
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this self.constituents[1] appropriate here, or would be better to check if parameter sublattice is contained in self.components?

I think this PR is ready or almost ready. Do you think that should I remove the excessive tests and keep just the test now named as test_model_energy?

@bocklund bocklund self-requested a review December 5, 2020 23:13
@bocklund
Copy link
Collaborator

bocklund commented Dec 5, 2020

I'm sorry @igorjrd. I totally lost track of the fact that you made changes here in May and it was on me to respond to them.

I took what was here and cleaned it up a little. The code in Model.py is effectively unchanged from @igorjrd implemented. I cleaned up the test cases as @richardotis suggested. After looking at them, they were mostly redundant tests on the same database and checking whether the site fraction symbols are in the Model instance AST is enough to exercise these changes.

The tests are here and are passing on all platforms (Coveralls is failing because @igorjrd's fork doesn't have our secrets) - we're paying that penalty for that PRs from forks, heh 😁.

I've requested reviews from both @igorjrd and @richardotis. Please let me know if you agree to merging this with your review!

@igorjrd
Copy link
Contributor Author

igorjrd commented Dec 6, 2020

Don't worry, Brandon. I didn't warned you about the changes. Also, in these last months it was difficult to me to provide new contributions on pycalphad, what turned hard to me to contact you about active PR's progress.

I checked the changes and agreed with them. I also have to say sorry about the redundant tests and thanks for removing the redundant. I think this PR is ready to be merged!

@bocklund bocklund merged commit 2a20a6d into pycalphad:develop Dec 6, 2020
@bocklund bocklund added this to the 0.8.5 milestone Jan 7, 2021
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.

Parameters with active components that are not in the Phase constituent array produce incorrect models
3 participants