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

changed jsr_run in FD to reach exactely tau #144

Merged
merged 1 commit into from
Aug 4, 2024
Merged

changed jsr_run in FD to reach exactely tau #144

merged 1 commit into from
Aug 4, 2024

Conversation

NellyMitnik
Copy link
Contributor

run_jsr in flux.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.

t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 19 lines in your changes missing coverage. Please review.

Project coverage is 47.19%. Comparing base (69dfe87) to head (9d2498a).

Files Patch % Lines
t3/utils/flux.py 0.00% 19 Missing ⚠️
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              
Flag Coverage Δ
unittests 47.19% <0.00%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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):
Copy link
Member

Choose a reason for hiding this comment

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

please add a docstring and typehints for each new function, e.g., as we have for run_jsr below

Copy link
Member

Choose a reason for hiding this comment

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

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])
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

was this addressed?

t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the tests label Jan 21, 2024
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks, looking great! only some very minor requests:

.vscode/settings.json Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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")
Copy link
Member

Choose a reason for hiding this comment

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

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):
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

was this addressed?

t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
t3/utils/flux.py Outdated Show resolved Hide resolved
@@ -2,6 +2,13 @@
"python.testing.pytestArgs": [
Copy link
Member

Choose a reason for hiding this comment

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

please uncommit the changes to this file

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")
Copy link
Member

Choose a reason for hiding this comment

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

please remove the prints in the test

@alongd
Copy link
Member

alongd commented May 2, 2024

@NellyMitnik , what's the status of this PR?

@alongd
Copy link
Member

alongd commented Jul 21, 2024

@NellyMitnik, I squashed and forced pushed. Can you review?

@alongd alongd merged commit 02a730d into main Aug 4, 2024
5 checks passed
@alongd alongd deleted the flux_jsr_check branch August 4, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants