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 SolutionArray extra slice #1204

Merged
merged 2 commits into from
Mar 3, 2022

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 21, 2022

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

Closes #1203

If applicable, provide an example illustrating new features this pull request is introducing

In [1]: import cantera as ct
   ...: import numpy as np
   ...: 
   ...: gas = ct.Solution("gri30.yaml")
   ...: 
   ...: T = np.arange(900, 1000, 50)
   ...: estimated_ignition_delay_times = np.ones_like(T) * 0.005
   ...: ignition_delays = ct.SolutionArray(
   ...:     gas, shape=T.shape, extra={"tau": estimated_ignition_delay_times}
   ...: )
   ...: ignition_delays.TP = T, 40.0 * 101325.0
   ...: for state in ignition_delays:
   ...:     print(state.tau)
0.005
0.005

Checklist

  • The pull request includes a clear description of this code change
  • Commit messages have short titles and reference relevant issues
  • Build passes (scons build & scons test) and unit tests address code coverage
  • Style & formatting of contributed code follows contributing guidelines
  • The pull request is ready for review

@ischoegl ischoegl changed the title Fix solutionarray extra slice Fix SolutionArray extra slice Feb 21, 2022
@ischoegl ischoegl requested a review from a team February 21, 2022 15:10
@ischoegl ischoegl force-pushed the fix-solutionarray-extra-slice branch from 35b7c31 to 3edbacd Compare February 25, 2022 23:11
@ischoegl
Copy link
Member Author

ischoegl commented Mar 1, 2022

@bryanwweber … this fix for the issue you reported is ready for review (and should be quick).

Copy link
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 @ischoegl, one case that I think needs to be investigated here.

As a total aside, outside the scope of this PR, but just to write it down somewhere, it'd be nice if SolutionArray supported:

  1. len()
  2. .shape
  3. Accessing extra keys that can't be accessed by attributed because they have an invalid character (-, , etc.)

interfaces/cython/cantera/composite.py Show resolved Hide resolved
@ischoegl ischoegl force-pushed the fix-solutionarray-extra-slice branch from 3edbacd to 9c1651d Compare March 3, 2022 21:20
@ischoegl ischoegl force-pushed the fix-solutionarray-extra-slice branch from 9c1651d to c7f0865 Compare March 3, 2022 21:30
Copy link
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! Once the tests pass this is good for me

assert state.index == ix

assert list(states[:2].index) == [0, 1]
assert list(states[100:102].index) == [] # outside of range
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this should raise an IndexError, but insofar as this maintains current behavior, it seems fine to me to keep it like this and defer adding the IndexError.

@ischoegl ischoegl merged commit 3a30e3b into Cantera:main Mar 3, 2022
@ischoegl ischoegl deleted the fix-solutionarray-extra-slice branch March 5, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Looping over a SolutionArray does not include extra items
2 participants