Fixed slicing update

[sin-ha]

Checklist

• There is a clear use-case for this code change
• The commit message has a short title & references relevant issues
• Build passes (scons build & scons test) and unit tests address code coverage
• The pull request is ready for review

If applicable, fill in the issue number this pull request is fixing

Fixes #804
The changes allow the required update in edge cases of list slicing in SolutionArray after squish

Changes proposed in this pull request

-Changes to accommodate list slicing by making two cases of the SolutionArray

[bryanwweber]

Another way to fix this is to change line 497 to say if states: instead of if states is not None:. I'm not sure what other implications that would have though. Do you have any idea which would be better?

[bryanwweber]

Also, can you please add some tests that check that this fix is not reverted by accident in the future? Same for your previous fix too.

[sin-ha]

Another way to fix this is to change line 497 to say if states: instead of if states is not None:. I'm not sure what other implications that would have though. Do you have any idea which would be better?

the same test fails for this it would need more tweaks I guess.

>>> states[1]    #arr[1] in the issue
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.7/site-packages/cantera/composite.py", line 553, in __getitem__
    return SolutionArray(self._phase, shape, states)
  File "/usr/local/lib/python3.7/site-packages/cantera/composite.py", line 497, in __init__
    if states:
ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Also, can you please add some tests that check that this fix is not reverted by accident in the future? Same for your previous fix too.

I thought there was one written and this was a subcase hence skipped this, Would write one now

[bryanwweber]

Thanks for the explanations @sin-ha! Can you edit the commit messages to make them something more meaningful? Thank you!

[sin-ha]

@bryanwweber yes sure, I just observed there is a typo in the comment would mend that too with it.

[speth]

I think the tests need to cover properties other than scalars that are independent of the slice. For instance, shouldn't all of these cases generate zero-length slices? I get some worrying output from taking such slices:

>>> gas = ct.Solution('h2o2.yaml')
>>> s = ct.SolutionArray(gas, 5)
>>> s.TP = [400,500,600,700,800], ct.one_atm
>>> s[:0].T
array(101325.)
>>> s[1:1].P
array(101325.)
>>> s[3:3].X
array([0.00e+000, 0.00e+000, 0.00e+000, 0.00e+000, 0.00e+000, 9.88e-321,
       0.00e+000, 0.00e+000, 4.94e-324])

[sin-ha]

@speth I guess the problem was with the default value of states. I guess the update should work.

[codecov]

Codecov Report

Merging #837 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #837   +/-   ##
=======================================
  Coverage   71.40%   71.40%           
=======================================
  Files         372      372           
  Lines       43605    43605           
=======================================
  Hits        31134    31134           
  Misses      12471    12471           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0089281...f64d22a. Read the comment docs.

[bryanwweber]

@sin-ha can you please rebase this branch onto the current tip of the master branch, and fix any conflicts? If you need some help with that, please let me know!

[sin-ha]

Yes I am getting some conflicts in Sconstruct which shouldn't be there while trying to rebase.
edit:
I think I merged the commits instead of rebasing the branch, apologies

[sin-ha]

@bryanwweber i tried but i cant find why would the travis ci build fail for Mac OS i guess my branch is properly rebased isn't it as i don't think the changes are critical to the build to collapse before starting as in the logs

[bryanwweber]

@sin-ha I restarted the job, it looked like it failed to install conda.

[bryanwweber]

Thanks @sin-ha! One small change here, then it's good from me.