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

[WIP] Jet Stirred Reactors #17

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Conversation

skrsna
Copy link

@skrsna skrsna commented Aug 15, 2019

  • Ready for review

Todo:

  • Multiprocessing
  • Efficently save .h5 files
  • Refactoring
    • eval_model.py
    • simulation.py
  • Error functions
    • Difference between area under the curves using numpy.trapz
  • Multiple datasets

@rwest
Copy link
Member

rwest commented Aug 16, 2019

I would suggest integrate the difference between the curves, rather than find the difference of the integrals.

@kyleniemeyer
Copy link
Member

I would suggest integrate the difference between the curves, rather than find the difference of the integrals.

I'm trying to visualize the difference here... in practice the answer should be the same, no? Though it's easier/more efficient to just integrate once.

@rwest
Copy link
Member

rwest commented Aug 16, 2019

I was imagining these two curves

Screen Shot 2019-08-16 at 4 27 15 PM

They have the same area under them, so the difference in area under curve is zero, suggesting they are identical and there is no error in the model.

If you find the difference between the curves, then the area between that and the y=0 axis (ok, so perhaps I should have said integrate the absolute value of the difference between the curves), the error is significant.

Screen Shot 2019-08-16 at 4 27 19 PM

@kyleniemeyer
Copy link
Member

@rwest got it, that makes sense!

@rwest
Copy link
Member

rwest commented Aug 16, 2019

It's by no means a perfect measure of fit. For example, these two pairs of curves would have the same "error".

Screen Shot 2019-08-16 at 4 33 20 PM

But it's a start, to get the code working.

@kyleniemeyer
Copy link
Member

kyleniemeyer commented Aug 16, 2019

Agreed.

FYI, when I was implementing the error evaluation for perfectly stirred reactor simulations in my older MARS, which marched along the upper branch, I used three points: near extinction turning point, near equilibrium (e.g., a residence time of around 0.1 s), and the logarithmic midpoint between those two points.

The overall error was the the max of the error in residence time at the extinction turning point and the error in temperature at the other two points.

elif self.kind == 'species profile':
timestep['mole_fractions'] = self.reac.thermo.X
# Add ``timestep`` to table
timestep.append()
Copy link
Member

Choose a reason for hiding this comment

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

JSR simulations needn't save time history.
Probably this whole method should be in the AutoIgnitionSimulation class, not the Simulation super-class

@rwest
Copy link
Member

rwest commented Jun 9, 2021

For our JSR simulations it's maybe worth reading 2E08.pdf and keeping an eye on Cantera/enhancements#31 and Cantera/cantera#1021

@sevyharris
Copy link

sevyharris commented Feb 24, 2022

I'm resurrecting this PR because I've got a flame speed addition in the works that depends on this.

Here's what I've done:

  • Finished adding multiprocessing
  • Added handling for multiple datasets
  • Made a bunch of PEP8/style changes because I can't help myself
  • Refactored the simulation files back into a single simulation.py file

Here's the remaining work:

  • Get existing tests back up and running
  • Add JSR tests
  • Refactor jsr_eval_model into eval_model

Here's what I don't plan to do for this PR:

  • Improve the error calculations. I made a curve_matching branch that can (in theory) handle this, but it deserves its own PR
  • Plotting

Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

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

Incomplete review; going to wait until refactoring finished. Not clear to me yet how multiple different simulation types are going to coexist and reuse appropriate code/classes.


# Local imports
from .utils import units
from .simulation import Simulation
from .simulation import AutoIgnitionSimulation as Simulation
Copy link
Member

Choose a reason for hiding this comment

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

In general I'm not sure I like the idea of importing something under a different name - could likely confuse people down the line. Is there a good reason for it?

@@ -40,7 +40,7 @@ def create_simulations(dataset, properties):
Returns
-------
simulations : list
List of :class:`Simulation` objects for each simulation
List of :class:`AutoignitionSimulation` objects for each simulation
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps I should wait to review until refactoring finished, but this file/module/class seems a little confused right now about if it's specific to Autoignition, or if it's generic for all simulations.

So I'm going to pause reviewing now. Let me know if/when I should take another look.

@sevyharris
Copy link

Okay, @rwest I think this one is ready for review.

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.

5 participants