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

Some fixes for IonFlame issues #912

Closed
wants to merge 3 commits into from
Closed

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Aug 9, 2020

Changes proposed in this pull request

  • Apply band-aid fix to ion_burner_flame.py - a reduction of domain size makes the example more stable; there is no apparent reason for simulations to fail on a larger domain, which points at underlying issues that should be fixed at a later point cherry-picked to Fix examples #905
  • Fix ch4_ion.yaml that did not match ch4_ion.cti cherry-picked to Fix examples #905
  • Fix apparent glitch that results in the Poisson solver not being activated (Edit: the fix has no effect as it affects C++ code that is not used)
  • Remove upstream von Neumann boundary condition for ionized species (see also Electron #700)
  • Add optional plots to IonFlow-based examples
  • Fix export of _other columns from IonFlow (i.e. grid, etc.) cherry-picked to Fix examples #905

Additional issues that surfaced while reviewing code are listed in #913 - changes are more extensive and should be handled by a separate PR (potentially after the release of 2.5).

PS: After cherry-picking 3 fixes into #905, the remaining changes are not worth keeping this PR open.

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

Fixes #909

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

@ischoegl
Copy link
Member Author

ischoegl commented Aug 9, 2020

@BangShiuh ... could you have a brief look at this (and the corresponding bug report in #911)?

@codecov
Copy link

codecov bot commented Aug 9, 2020

Codecov Report

Merging #912 into main will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #912      +/-   ##
==========================================
+ Coverage   71.19%   71.21%   +0.02%     
==========================================
  Files         376      376              
  Lines       46211    46199      -12     
==========================================
+ Hits        32898    32900       +2     
+ Misses      13313    13299      -14     
Impacted Files Coverage Δ
src/oneD/IonFlow.cpp 74.46% <ø> (+9.43%) ⬆️
src/base/AnyMap.cpp 83.99% <0.00%> (-0.05%) ⬇️
test/general/test_containers.cpp 99.57% <0.00%> (-0.02%) ⬇️

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 a9a3a4a...309429d. Read the comment docs.

@ischoegl
Copy link
Member Author

ischoegl commented Aug 10, 2020

On further inspection, the Python attribute electric_field_enabled (as well as the C++ counterparts) has no effect on the actual solution - the implementation is incomplete as the C++ vector m_do_electric_field is set but never used. Edit: this behavior is now reported as part of #913 - I am not planning to address it here.

@ischoegl ischoegl marked this pull request as draft August 10, 2020 12:56
@ischoegl ischoegl changed the title Fix Poisson solver activation for IonFlameBase Some fixes for IonFlame issues Aug 10, 2020
@ischoegl ischoegl mentioned this pull request Aug 10, 2020
4 tasks
@ischoegl ischoegl marked this pull request as ready for review August 10, 2020 13:21
@ischoegl
Copy link
Member Author

ischoegl commented Aug 10, 2020

@bryanwweber ... I added plots to the examples, and fixed another glitch for the SolutionArray export of IonFlow. At this point, I think this PR ready for review.

@ischoegl ischoegl force-pushed the enable-poisson branch 3 times, most recently from 3793288 to 279e0d1 Compare August 10, 2020 19:55
The change corrects an apparent glitch in the code, making sure that the C++
IonFlow::m_do_electric_field booleans are set correctly. As the C++ flags
remain unused, the change has no effect on the Poisson solver.
@ischoegl
Copy link
Member Author

After cherry-picking 3 fixes into #905, the remaining changes are not worth keeping this PR open.

@ischoegl ischoegl closed this Aug 20, 2020
@ischoegl ischoegl deleted the enable-poisson branch January 29, 2021 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flawed upstream boundary condition in 1D IonFlow?
1 participant