changed jsr_run in FD to reach exactely tau #144
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
- Coverage 47.36% 47.19% -0.17%
==========================================
Files 25 25
Lines 3393 3405 +12
Branches 914 917 +3
==========================================
Hits 1607 1607
- Misses 1583 1595 +12
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @NellyMitnik
Please see some comments below. Feel free to squash the commits together. Also, could you add a test to the new function you added and an overall test for this feature?
t3/utils/flux.py
Outdated
| t = 0 | ||
| while t < tau: | ||
| dt = tau/num_steps |
There was a problem hiding this comment.
minor: please add a space before and after operators, e.g. tau / num_steps
Also below for tau*tau_tolerance and other cases
t3/utils/flux.py
Outdated
| @@ -295,6 +295,15 @@ def set_batch_p(gas: ct.Solution, | |||
| network.rtol = r_tol | |||
| return network, reactor | |||
|
|
|||
| def closest_bigger_number(array, target): | |||
There was a problem hiding this comment.
please add a docstring and typehints for each new function, e.g., as we have for run_jsr below
There was a problem hiding this comment.
Was this comment addressed? (please add the docstring in the same commit as the function)
t3/utils/flux.py
Outdated
| candidates = [(num, i) for i, num in enumerate(array) if num > target] | ||
| # Find the closest bigger number | ||
| if candidates: | ||
| m, i = min(candidates, key=lambda x: x[0]) |
There was a problem hiding this comment.
Can you explain what is being returned as m, i? Looks to me like the min() func only returns one value, I'm probably missing something
There was a problem hiding this comment.
Let's use consistent variable names, for example if we call the content of the tuple pair (num, i), then let's also call them num, i on this line if indeed they represent the same type of data
There was a problem hiding this comment.
Candidates is a list of tuples (num, i), with numbers from the original array which are bigger that the target number. Let's call this process as filtering out smaller numbers.
In the next step, the min function is used over a list of tuples, while the key for the min function is the first variable in tuple (which is the value number itself). The min function returns a value from the list (which is a tuple consists of a value and its index in the original array).
t3/utils/flux.py
Outdated
| @@ -304,6 +313,7 @@ def run_jsr(gas: ct.Solution, | |||
| V: Optional[float] = None, | |||
| a_tol: float = 1e-16, | |||
| r_tol: float = 1e-10, | |||
| tau_tolerance: float = 0.1, | |||
There was a problem hiding this comment.
Are we sure that 0.1 isn't too risky? In the PR comment you wrote that it reached from 0.8 to 8 sec. So if next time it will jump from 0.9 to 8 sec it would be enough for the 0.1 tolerance to miss this jump in t. We need the tolerance just for the lower range, let's make it ~0.01 or 0.005?
NellyMitnik
force-pushed
the
flux_jsr_check
branch
from
222be9d to
e941a80
Compare
There was a problem hiding this comment.
Thanks @NellyMitnik!
Please squash the commits that are fixups
I added some comments, some of them were probably addressed by the fixups, but please take a look
t3/utils/flux.py
Outdated
| t = tau | ||
| t = time_steps_array[t_step_index] | ||
| network.advance(time_steps_array[t_step_index]) | ||
| print(network.time) |
There was a problem hiding this comment.
Should this print statement be removed?
t3/utils/flux.py
Outdated
| @@ -351,6 +355,7 @@ def run_jsr(gas: ct.Solution, | |||
| rops[spc.name][rxn.equation] += cantera_reaction_rops[i] * stoichiometry[spc.name][i] | |||
| profile = {'P': gas.P, 'T': gas.T, 'X': {s.name: x for s, x in zip(gas.species(), gas.X)}, 'ROPs': rops} | |||
| if t == tau: | |||
| print("exactly tau") | |||
There was a problem hiding this comment.
same here, I think this print should be removed
t3/utils/flux.py
Outdated
| @@ -295,6 +295,15 @@ def set_batch_p(gas: ct.Solution, | |||
| network.rtol = r_tol | |||
| return network, reactor | |||
|
|
|||
| def closest_bigger_number(array, target): | |||
There was a problem hiding this comment.
Was this comment addressed? (please add the docstring in the same commit as the function)
t3/utils/flux.py
Outdated
| @@ -304,6 +313,7 @@ def run_jsr(gas: ct.Solution, | |||
| V: Optional[float] = None, | |||
| a_tol: float = 1e-16, | |||
| r_tol: float = 1e-10, | |||
| tau_tolerance: float = 0.1, | |||
.vscode/settings.json
Outdated
| @@ -2,6 +2,13 @@ | |||
| "python.testing.pytestArgs": [ | |||
There was a problem hiding this comment.
please uncommit the changes to this file
tests/test_flux.py
Outdated
| num, i = flux.closest_bigger_number([1, 2, 2.5, 3, 3.5, 4.5, 5], 3.7) | ||
| assert num == 4.5 | ||
| assert i == 5 | ||
| print("\npassed test case 1") |
There was a problem hiding this comment.
please remove the prints in the test
|
@NellyMitnik , what's the status of this PR? |
|
@NellyMitnik, I squashed and forced pushed. Can you review? |
run_jsrinflux.py, was passing tau by a huge factor, and then, it was time-integrating backward to tau.For example, if tau = 2 sec, by stepping, it reached from 0.8 to 8 sec. When reaching 8 sec, it advanced to 2 sec (time-integrating backward).
To fix that, an array of 500 steps in time was defined. A loop iterates over this array and advances by equal time-steps until it reaches exactly tau.