-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Fixed slicing update #837
Fixed slicing update #837
Conversation
There was a problem hiding this 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?
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. |
the same test fails for this it would need more tweaks I guess.
I thought there was one written and this was a subcase hence skipped this, Would write one now |
Thanks for the explanations @sin-ha! Can you edit the commit messages to make them something more meaningful? Thank you! |
@bryanwweber yes sure, I just observed there is a typo in the comment would mend that too with it. |
self.assertEqual(arr2.n_species, 9) | ||
|
||
arr3 = soln[None:0] | ||
self.assertEqual(arr3.n_reactions, 28) |
There was a problem hiding this comment.
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])
There was a problem hiding this comment.
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 Report
@@ 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.
|
@sin-ha can you please rebase this branch onto the current tip of the |
Yes I am getting some conflicts in Sconstruct which shouldn't be there while trying to rebase. |
@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 |
@sin-ha I restarted the job, it looked like it failed to install conda. |
There was a problem hiding this 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.
Checklist
scons build
&scons test
) and unit tests address code coverageIf 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