Closed david-zwicker closed 2 months ago
I currently base the new Phases
class on the tuple approach that we had so far, so the class works as a drop-in replacement. However, we could also decide to get rid of the tuples completely (particularly in the examples) and base everything on class access.
Attention: Patch coverage is 84.78261%
with 7 lines
in your changes missing coverage. Please review.
Files with missing lines | Patch % | Lines |
---|---|---|
flory/common/phases.py | 82.92% | 7 Missing :warning: |
Flag | Coverage Δ | |
---|---|---|
Tests | 68.78% <84.78%> (-2.12%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files with missing lines | Coverage Δ | |
---|---|---|
flory/mcmp/_finder_impl.py | 99.17% <ø> (+0.63%) |
:arrow_up: |
flory/mcmp/finder.py | 71.42% <100.00%> (-2.65%) |
:arrow_down: |
flory/shortcut.py | 100.00% <100.00%> (ø) |
|
flory/common/phases.py | 82.92% <82.92%> (ø) |
We should also discuss whether we should call the fields Js
and phis
or rather volumes
and fractions
or something similar.
I like this! For exposing the clustering threshold, can we let the Phases class also hold the original phis and Js as private members? Then we can re-cluster the phases when needed.
Let's discuss this in person some time this week!
I hope this avoids errors (so one does not need to remember whether the phase volume was the first or second entry). The class also allows to add convenience methods and some checks if we want this later.