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: pure vacancy phase detection #239

Merged
merged 3 commits into from Sep 25, 2019
Merged

Conversation

igorjrd
Copy link
Contributor

@igorjrd igorjrd commented Sep 15, 2019

Bugfix to detect phases containing pure vacancy sublattices, according to: #191

When any phase satisfies the requirement (containing at least one pure vacancy sublattice) an error message is raised.

# None of the components in a sublattice are active
# We cannot build a model of this phase
raise DofError(
'{0}: Sublattice {1} of {2} has no components in {3}' \
.format(self.phase_name, sublattice,
phase.constituents,
self.components))
self.constituents.append(set(sublattice).intersection(self.components))
if sum(set(map(lambda s : getattr(s, 'number_of_atoms'),sublattice_comps))) == 0:
Copy link
Collaborator

@bocklund bocklund Sep 15, 2019

Choose a reason for hiding this comment

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

Thanks for submitting this PR!

I think this line may be causing the tests to fail because any sublattice that has only VA will raise an error, but we only want to raise an error here if all sublattices have only VA.

For example, given a phase with sublattices (('FE', 'VA'), ('VA')), the phase should built if both FE and VA are specified (i.e. it's ok if the last sublattice is VA only, as long as the first sublattice has FE occupied).

It might be helpful to write a unit test that exercises some different conditions.

Copy link
Contributor Author

@igorjrd igorjrd Sep 15, 2019

Choose a reason for hiding this comment

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

You're welcome! Its a pleasure to try contributing on pycalphad.

I see, as you said, my previous approach works detecting if the phase has any pure vancacy sublattice. I fixed this storing the number_of_atoms of each sublattice in a set called is_pure_VA and checking if the sum of is_pure_VA is equal to zero after get the number_of_atoms in each sublattice. Now i think that it will work, I tested building some Models used in the tests of TravisCI and it worked fine

@bocklund
Copy link
Collaborator

bocklund commented Sep 18, 2019

The builds all seem to be passing (appveyor checked by hand because of #238), except for some non-test related bug where pip wants to install certifi over the conda installed version on macOS+Python 3.6. This is also happening over at #240. This looks ready to merge to me.

Copy link
Collaborator

@richardotis richardotis left a comment

This looks great. Two minor changes before we merge:

  1. Please delete the extra whitespace which was added at the end of model.py (not a big deal, but since we have to make the other change anyway)
  2. Can we add the test case to pycalphad/tests/test_model.py?

@richardotis richardotis merged commit abccdf5 into pycalphad:develop Sep 25, 2019
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.

None yet

3 participants