Skip to content

Fixed slicing update#837

Merged
speth merged 2 commits intoCantera:masterfrom
sin-ha:fix804
Jun 5, 2020
Merged

Fixed slicing update#837
speth merged 2 commits intoCantera:masterfrom
sin-ha:fix804

Conversation

@sin-ha
Copy link
Copy Markdown
Contributor

@sin-ha sin-ha commented Mar 30, 2020

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
bryanwweber requested changes Mar 30, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Member

bryanwweber commented Mar 30, 2020

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
Copy link
Copy Markdown
Contributor Author

sin-ha commented Mar 30, 2020 •
edited
Loading

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
Copy link
Copy Markdown
Member

bryanwweber commented Apr 1, 2020

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

@sin-ha
Copy link
Copy Markdown
Contributor Author

sin-ha commented Apr 1, 2020

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

speth
speth requested changes Apr 2, 2020
View reviewed changes
interfaces/cython/cantera/test/test_thermo.py Outdated
self.assertEqual(arr2.n_species, 9)

arr3 = soln[None:0]
self.assertEqual(arr3.n_reactions, 28)
Copy link
Copy Markdown
Member

@speth speth Apr 2, 2020

Choose a reason for hiding this comment

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

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])

Copy link
Copy Markdown
Contributor Author

@sin-ha sin-ha Apr 8, 2020

Choose a reason for hiding this comment

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

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

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2020 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           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
Copy link
Copy Markdown
Member

bryanwweber commented Apr 8, 2020

@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
Copy link
Copy Markdown
Contributor Author

sin-ha commented Apr 8, 2020 •
edited
Loading

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
Copy link
Copy Markdown
Contributor Author

sin-ha commented Apr 9, 2020 •
edited
Loading

@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
Copy link
Copy Markdown
Member

bryanwweber commented Apr 9, 2020

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

bryanwweber
bryanwweber requested changes May 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@bryanwweber bryanwweber left a comment

Choose a reason for hiding this comment

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

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

@speth speth merged commit 437d870 into Cantera:master Jun 5, 2020
12Chao pushed a commit to 12Chao/cantera that referenced this pull request Jul 1, 2020
@sin-ha
Fix zero-length slicing in SolutionArray
d51b84f 
Fixes Cantera#837
srikanthallu pushed a commit to srikanthallu/cantera that referenced this pull request Sep 17, 2020
@sin-ha @srikanthallu
Fix zero-length slicing in SolutionArray
ed58cb1 
Fixes Cantera#837
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@speth speth speth approved these changes

@bryanwweber bryanwweber bryanwweber approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slicing of SolutionArray raises error

3 participants

@sin-ha @bryanwweber @speth