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

OneD domain factories and cleanup #1445

Merged
merged 24 commits into from
Mar 18, 2023
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Feb 28, 2023

Changes proposed in this pull request

  • Implement DomainFactory (see https://github.com/Cantera/cantera/projects/2)
  • Initial simplifications in Python API
  • Deprecate magic numbers in Domain1D::domainType
  • Replace raw pointers by shared pointers in assembly of Sim1D.
  • Remove solution from Domain1D::show and Sim1D::show to avoid erroneous conflation with Solution objects
  • Introduce clib function returning domain type as well as new_domain factory loader (to facilitate Matlab toolbox revamp #1182)

While other tweaks are possible, they will create merge conflicts with #1079

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

Implements one 'idea' in https://github.com/Cantera/cantera/projects/2

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

shared_ptr<Domain1D> inlet = newDomain("inlet", sol, "reactants");
shared_ptr<Domain1D> flow = newDomain("gas-flow", sol, "flow");

for clib, the following items are added:

int domain_type3(int i, size_t lennm, char* nm); /* returns char* instead of magic number */
int sim1D_show(int i, const char* fname); /* replaces sim1D_showSolution */
int bdry_setSpreadRate(int i, double v); /* replaces inlet_setSpreadRate */
double bdry_spreadRate(int i); /* add missing getter */

int soln_setTransportModel(int n, const char* model); /* addendum to #1448 */
int trans_transportModel(int n, int lennm, char* nm); /* returns char* specifying transport type */

int domain_new(const char* type, int i, const char* id); /* factory constructor */

For the Python API, the following items are (re-)introduced:

flow = ct.FreeFlow(gas)
flow = ct.AxisymmetricFlow(gas)

with IdealGasFlow being deprecated.

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

@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #1445 (6d84d7f) into main (0c5fc1d) will decrease coverage by 0.05%.
The diff coverage is 61.83%.

@@            Coverage Diff             @@
##             main    #1445      +/-   ##
==========================================
- Coverage   69.86%   69.81%   -0.05%     
==========================================
  Files         373      375       +2     
  Lines       56684    56872     +188     
  Branches    18883    18980      +97     
==========================================
+ Hits        39600    39705     +105     
- Misses      14571    14650      +79     
- Partials     2513     2517       +4     
Impacted Files Coverage Δ
include/cantera/oneD/IonFlow.h 84.61% <ø> (ø)
include/cantera/oneD/OneDim.h 53.62% <ø> (ø)
include/cantera/oneD/Sim1D.h 66.66% <ø> (-3.34%) ⬇️
src/clib/ctmultiphase.cpp 0.00% <0.00%> (ø)
src/oneD/IonFlow.cpp 49.14% <0.00%> (-1.15%) ⬇️
src/oneD/Domain1D.cpp 71.59% <15.38%> (-4.63%) ⬇️
src/oneD/Boundary1D.cpp 55.39% <16.66%> (-0.03%) ⬇️
include/cantera/oneD/Boundary1D.h 48.97% <33.33%> (-6.58%) ⬇️
src/clib/ctonedim.cpp 20.91% <37.50%> (+0.02%) ⬆️
include/cantera/oneD/Domain1D.h 79.23% <40.00%> (-1.54%) ⬇️
... and 12 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ischoegl ischoegl force-pushed the oneD-domain-factories branch 3 times, most recently from cc142ab to aacc987 Compare February 28, 2023 17:49
@ischoegl ischoegl changed the title OneD domain factories OneD domain factories and cleanup Feb 28, 2023
@ischoegl ischoegl force-pushed the oneD-domain-factories branch 3 times, most recently from 7cdd0dc to 92dfb97 Compare February 28, 2023 21:58
@ischoegl ischoegl marked this pull request as ready for review February 28, 2023 22:31
@ischoegl
Copy link
Member Author

@speth ... after running into #1447 (which likely involves tackling Cantera/enhancements#158) I am omitting factory constructors in clib for the moment.

@ischoegl ischoegl marked this pull request as draft March 3, 2023 03:06
@ischoegl
Copy link
Member Author

ischoegl commented Mar 3, 2023

Converting back to draft, as this should be merged after #1448.

@ischoegl
Copy link
Member Author

ischoegl commented Mar 7, 2023

Rebased after the merge of #1448; some experiments for the introduction of SharedCabinet and/or newDomain factory constructors indicate that updates to clib/ctonedim will have to be somewhat more extensive than anticipated, and are postponed for another time. Edit: changes are now incorporated.

@ischoegl ischoegl marked this pull request as ready for review March 7, 2023 04:09
@ischoegl ischoegl requested a review from a team March 7, 2023 04:10
@ischoegl ischoegl force-pushed the oneD-domain-factories branch 6 times, most recently from d78871b to 3d533ab Compare March 13, 2023 00:34
@ischoegl
Copy link
Member Author

Resolved a minor merge conflict introduced by #1426.

Copy link
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.

Thanks, @ischoegl. This looks very good, and I think having the 1D code start to use the factory infrastructure that we have for most other object types will simplify some future additions (say, ExtensibleDomain). I had only a handful of minor suggestions.

interfaces/cython/cantera/_onedim.pyx Outdated Show resolved Hide resolved
interfaces/cython/cantera/onedim.py Show resolved Hide resolved
interfaces/cython/cantera/onedim.py Show resolved Hide resolved
src/clib/Cabinet.h Show resolved Hide resolved
src/clib/ct.cpp Show resolved Hide resolved
src/clib/ctmultiphase.cpp Outdated Show resolved Hide resolved
src/oneD/StFlow.cpp Show resolved Hide resolved
test/clib/test_clib.cpp Outdated Show resolved Hide resolved
@ischoegl ischoegl force-pushed the oneD-domain-factories branch 3 times, most recently from 1dacf88 to 6624f6c Compare March 18, 2023 18:51
@ischoegl
Copy link
Member Author

ischoegl commented Mar 18, 2023

@speth ... thanks for the review! I adopted some suggestions, and responded to others. Edit: I removed StFlow::isFree as I don't think that a new methods should be introduced.

Copy link
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.

Thanks for the updates, @ischoegl. This looks good to me.

@speth speth merged commit 2884a08 into Cantera:main Mar 18, 2023
@ischoegl ischoegl deleted the oneD-domain-factories branch March 18, 2023 21:35
ssun30 added a commit to ssun30/cantera that referenced this pull request Apr 21, 2023
ssun30 added a commit to ssun30/cantera that referenced this pull request May 8, 2023
speth pushed a commit to ssun30/cantera that referenced this pull request May 11, 2023
speth pushed a commit that referenced this pull request May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

2 participants