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: Calculating disordered phase only if respective ordered phase inactive #248

Conversation

@igorjrd
Copy link
Contributor

igorjrd commented Oct 18, 2019

Fixing the bug presented at #247
A new function check_order_disorder is now responsible to exclude the disordered phases when its respective ordered phase are active.

Some tests in test_utils had to be slightly changed, to run the new function check_order_disorder to remove the disordered phases when active phases are active.

igorjrd added 5 commits Oct 17, 2019
@richardotis richardotis self-assigned this Oct 21, 2019
@richardotis richardotis self-requested a review Oct 28, 2019
Copy link
Collaborator

richardotis left a comment

Great work!

Three comments:

  1. check_order_disorder should be combined with (or called by) filter_phases because it is part of that function's job.
  2. Then you can change the tests back to how they were, since the API is now unchanged.
  3. Finally, please use one of Brandon's test cases in #247 to create a new test to make sure the fix is working.
@igorjrd

This comment has been minimized.

Copy link
Contributor Author

igorjrd commented Oct 30, 2019

@richardotis, I will deprecate the proposed check_order_disorder combining this on filter_phases. To turn possible calculating disordered phases when its respective ordered phases are inactive, filter_phases will need to take the phases entried by the user as argument. As consequence, the tests will need to be reedited to add phases argument to filter_phases. Does this propose seems good?

@richardotis

This comment has been minimized.

Copy link
Collaborator

richardotis commented Oct 30, 2019

@bocklund Any downstream implications for making this API change to filter_phases?

@bocklund

This comment has been minimized.

Copy link
Collaborator

bocklund commented Oct 30, 2019

I don't have any code that would be affected by this, however I have used filter_phases as way to get all the possible active phases.

One option is to preserve that behavior and use a specific subset of phases as candidates, e.g. filter_phases(dbf, comps, candidate_phases=None) where the absence of any candidate phases gives the same result as filter_phases currently does (i.e. the default is filter_phases(dbf, comps, dbf.phases.keys())), but having a set of candidate_phases that includes the disordered phase, but does not include the ordered phase in an order-disorder model correctly preserves the disordered phase.

@richardotis

This comment has been minimized.

Copy link
Collaborator

richardotis commented Oct 30, 2019

@igorjrd Please move forward with your plan. I recommend following Brandon's suggestion on using a new keyword argument in filter_phases.

pycalphad/core/calculate.py Outdated Show resolved Hide resolved
pycalphad/core/equilibrium.py Outdated Show resolved Hide resolved
@richardotis richardotis merged commit 6722a58 into pycalphad:develop Nov 5, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 86.159%
Details
@richardotis

This comment has been minimized.

Copy link
Collaborator

richardotis commented Nov 5, 2019

Thanks again for your work @igorjrd

bocklund added a commit that referenced this pull request Dec 5, 2019
…he class name (#250)

* FIX: Calculating disordered phase only if respective ordered phase inactive (#248)

Fixes #247.
A new function `check_order_disorder` is now responsible for excluding the disordered phases when its respective ordered phase is active.

Some tests in test_utils had to be slightly changed, to run the new function `check_order_disorder` to remove the disordered phases when corresponding ordered phases are active.

* DOC: API documentation for pycalphad.plot.binary module

* CHANGES: 0.8.1

* BLD: meta.yaml: Fix incorrect dependency spec

* BLD: setup.py: Add pycalphad.plot.binary to packages

* Rewrite pointsolver to require passage of the problem instance, not the
class name.

* Point solver API

* Rewrite pointsolver to require passage of the problem instance, not the
class name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.