Skip to content

Add ability to sort SolutionArray objects#688

Merged
speth merged 2 commits intoCantera:masterfrom
ischoegl:add-ability-to-sort-solution-array
Oct 25, 2019
Merged

Add ability to sort SolutionArray objects#688
speth merged 2 commits intoCantera:masterfrom
ischoegl:add-ability-to-sort-solution-array

Conversation

@ischoegl
Copy link
Copy Markdown
Member

@ischoegl ischoegl commented Aug 10, 2019 •
edited
Loading

Please fill in the issue number this pull request is fixing:

Fixes #625

Changes proposed in this pull request:

  • implements sort method for 1D SolutionArray objects
  • adds some simple type checks for an instantiation with states not None

Intended usage:

# create SolutionArray and add items with `tau` not following an increasing order
states = ct.SolutionArray(gas, extra=('tau',))
...
states.sort('tau')

Some observations:

  • Sorting of higher-dimensional SolutionArray objects is not clearly defined
  • Difference/necessity of apparently duplicate SolutionArray._extra_lists and SolutionArray._extra_arrays dictionaries is not clear
  • SolutionArray._extra_lists is apparently only used in SolutionArray.__call__ (docstring is missing ... it works as copying constructor that extracts species, but usage is not clear)

@ischoegl ischoegl force-pushed the add-ability-to-sort-solution-array branch from e9a4629 to e443232 Compare August 10, 2019 02:36
@ischoegl ischoegl mentioned this pull request Aug 10, 2019
Closed
@bryanwweber
Copy link
Copy Markdown
Member

bryanwweber commented Aug 10, 2019

Does it make sense to add a kwarg to reverse the sort? Or does the [::-1] indexing work to reverse the SolutionArray?

@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Aug 10, 2019

[::-1] does not work, but implementing the kwarg is trivial. Can add this if there's a need.

@ischoegl ischoegl force-pushed the add-ability-to-sort-solution-array branch 5 times, most recently from 2728a6f to d5aec44 Compare August 13, 2019 17:10
@ischoegl ischoegl force-pushed the add-ability-to-sort-solution-array branch from d5aec44 to 62b550d Compare October 3, 2019 03:06
@Cantera Cantera deleted a comment from codecov bot Oct 3, 2019
@Cantera Cantera deleted a comment from codecov bot Oct 3, 2019
speth
speth reviewed Oct 25, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

I realize this wasn't well documented, but the duplication of data between _extra_arrays and _extra_lists was meant as an optimization for accessing the "extra" variables. Data is initially stored in the list form so that appending to the SolutionArray is efficient. However, we want to return an array when those variables are accessed, and at the time I initially wrote this, I wanted to avoid the inefficiency of repeatedly converting the list to an array in the case where the data hadn't changed.

However, with the expansion of the features of the SolutionArray class, I think maintaining the duplicate data isn't worth the effort, so keeping just the list-based storage makes sense. It might be worth renaming it just _extra in that case, since there is no need to distinguish between that and the now-removed array version.

@ischoegl ischoegl force-pushed the add-ability-to-sort-solution-array branch from 62b550d to 85c2eba Compare October 25, 2019 02:26
@ischoegl
Copy link
Copy Markdown
Member Author

ischoegl commented Oct 25, 2019 •
edited
Loading

Data is initially stored in the list form so that appending to the SolutionArray is efficient.

That's how I interpreted this, and thought that np.array really only made sense for higher dimensions (which I don't believe need to be pre-empted at this point). Either way, I don't think it's necessary to keep duplicate data, and am happy that this can be simplified.

It might be worth renaming it just _extra in that case, since there is no need to distinguish between that and the now-removed array version.

Done.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 25, 2019 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #688   +/-   ##
=======================================
  Coverage   70.85%   70.85%           
=======================================
  Files         374      374           
  Lines       43668    43668           
=======================================
  Hits        30939    30939           
  Misses      12729    12729
Impacted Files Coverage Δ
src/transport/GasTransport.cpp 90.58% <0%> (ø) ⬆️

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 f02ca90...85c2eba. Read the comment docs.

@ischoegl ischoegl requested a review from speth October 25, 2019 02:54
@speth speth merged commit 1e01054 into Cantera:master Oct 25, 2019
@ischoegl ischoegl deleted the add-ability-to-sort-solution-array branch December 16, 2019 15:56
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

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add ability to sort SolutionArray

3 participants

@ischoegl @bryanwweber @speth