Changes for SE and battery models #247
Conversation
…type of switches for stats
… found if no adaptivity is used
|
There seems to be a bit code duplication going on (esp. within |
|
Then, how to make a generic class with N condensators to replace all the others? Sure, you'd need to have some |
|
Failing test for MacOS was a hickup. Failing test for Gitlab is another issue, not your concern. |
brownbaerchen
left a comment
brownbaerchen
left a comment
There was a problem hiding this comment.
I think the switch estimator has improved quite a bit in terms of abstraction since the last PR, well done!
However, I still think that the tests are way too coarse. While hardcoding desired results is a bit of a mess, I think it is crucial to make sure the code still does what you want it to tomorrow. Please look for a few numbers that are really specific to your problem. I think the most obvious candidate is when you find the switch(es). Also, the number restarts is an interesting and easy thing to check. Or the total number of iterations (with recomputed=None) would be interesting, since that is a variable determined by the interplay between the switch estimator and adaptivity and that is crucial for performance.
Sometimes, I felt like documentation could be a bit more elaborate, although that is something that we all (save for Thibaut maybe) could improve on. For readability I would suggest adding blank lines between the function description and the Args: and Returns: sections of the docstring.
I agree with Robert that there is ample opportunity for abstraction in the problem classes.
|
|
||
| else: | ||
| f.expl[0] = 0 | ||
| if self.t_switch is not None: |
There was a problem hiding this comment.
You could streamline this a bit by doing sth like
t_switch = np.inf if self.t_swtich is None else self.t_switch
if u[1] <= self.params.V_ref or t >= t_switch:
f.expl[0] = [...]
else:
f.expl[0] = 0
I think, in addition to repeating fewer lines, this would improve accuracy. What if the switch estimator estimates the switch slightly too late? Then, we have self.t_switch != None and enter the first condition, but also u[1] might be smaller than self.params.V_ref while also t < self.t_switch. In that case the problem does not switch, but I think you would want it too? As always, I am just guessing here...
| t (float): current time | ||
| Returns: | ||
| switch_detected (bool): Indicates if a switch is found or not | ||
| m_guess (np.int): Index of where the discrete event would found |
There was a problem hiding this comment.
I don't understand the documentation for m_guess
| vC_switch = [] | ||
| if switch_detected: | ||
| for m in range(1, len(u)): | ||
| vC_switch.append(u[m][1] - self.params.V_ref) |
There was a problem hiding this comment.
Since u is a numpy array, why not just vC_switch = u[1:, 1] - self.params.V_ref if switch_detected else []?
| non_f[0] = 0 | ||
|
|
||
| else: | ||
| if self.t_switch is not None: |
There was a problem hiding this comment.
Same as above, although this point is outdated with Robert's suggestion of merging the problem classes, I think.
| self.t_switch = self.get_switch(t_interp, vC_switch, m_guess) | ||
| self.params.t_switch = self.get_switch(t_interp, vC_switch, m_guess) | ||
|
|
||
| # if the switch is not find, we need to do ... ? |
There was a problem hiding this comment.
if the switch is not found, we need to do ... ?
| if not np.isclose(self.params.t_switch - L.time, L.dt, atol=self.params.tol): | ||
| dt_switch = self.params.t_switch - L.time | ||
| self.log( | ||
| f"Located Switch at time {self.params.t_switch:.6f} is outside the range of tol={self.params.tol:.4f}", |
There was a problem hiding this comment.
I am guessing the tolerance is sth like 1e-3. For such numbers you can use e instead of f in string formatting to get the exponent representation.
| dt_search = self.t_switch - L.time | ||
| L.prob.params.set_switch[self.count_switches] = self.switch_detected | ||
| L.prob.params.t_switch[self.count_switches] = self.t_switch | ||
| dt_switch = self.params.t_switch - L.time |
There was a problem hiding this comment.
This line appears in both the true and false parts of the if statement. Why not put it above the if statement?
| self.switch_detected_step = False | ||
| if self.params.switch_detected_step: | ||
| if L.time + L.dt >= self.params.t_switch: | ||
| self.params.t_switch = None |
There was a problem hiding this comment.
There is a function you can overload here with resetting of status variables. It doesn't change the functionality I would hope, but I find it cleans the code up to write everything in a function that does exactly what it is supposed to and nothing else. Although this is personal preference, as becomes evident when looking though the controller module for instance. ;)
|
|
||
| L.status.dt_new = self.dt_initial | ||
| dt_planned = L.status.dt_new if L.status.dt_new is not None else L.params.dt | ||
| L.status.dt_new = dt_planned |
There was a problem hiding this comment.
You could write the above two lines into one line
…_restructure Multiple Hooks simultaneously
|
I added a battery model using N condensators, where N is an arbitrary number > 0. I put it in Battery.py and removed Battery_2condensators.py to have not to many files for battery models. One file is enough. I hope it is as you imagined it @pancetta ? |
|
Something odd is going on here.. I see 30 files that changed, incl. ones that changed during the two major updates to this PR. It looks like the history is messed up, no? If so, please clean up the PR or (easier) create a new one with your changes. Sorry for the hassle, don't know what's causing this. Do you @brownbaerchen ? |
|
The same thing happened to Thibaut and me: We merge upstream changes, but GitHub doesn't compare to the new upstream. Instead the PR looks like it is compared to the upstream repo at the time the PR was started. |
|
It's maybe the same mistake @brownbaerchen and I already did : merging upstream/master into our branch. This could be solved probably on @lisawim's side by doing a merge of upstream/master into her own origin/master, and then merge of her own origin/master into SE_new ... cf : What is suggested for release branch in https://github.com/tlunet/pySDC/blob/contrib-2/docs/contrib/01_pull_requests.md, maybe we have to do the same for master ... ? |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #247 +/- ##
=========================================
Coverage ? 68.54%
=========================================
Files ? 225
Lines ? 19556
Branches ? 0
=========================================
Hits ? 13405
Misses ? 6151
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
brownbaerchen
left a comment
brownbaerchen
left a comment
There was a problem hiding this comment.
I only glanced over the changes, because I feared the PR would be maybe closed and reopened faster than I can check the conversations.
I really like the changes! The tests are really meaningful now!
At various points I made suggestions for merging similar code from different files, but since I only glanced over everything, I am unsure if it really can be merged without issue. Like the problem classes in particular. Maybe they are completely distinct after all.
| return me | ||
|
|
||
|
|
||
| class battery_n_condensators(ptype): |
There was a problem hiding this comment.
What is the difference between this class and the battery class? Both take ncondensators as essential keys.
There was a problem hiding this comment.
Hm. Battery has only one capacitor. And this one has any number of capacitors. I am asking myself why I used the german word and put in an englisch writing sense named condensators instead of writing capacitors.. Facepalming.. Anyway. Maybe I can say that battery_n_condensators inherits from battery, but I need to add almost each method instead of count_switches()
| ), f'{msg} Expected {key}={expected[key]:.4e}, got {key}={got[key]:.4e}' | ||
|
|
||
|
|
||
| def get_data_dict(stats, use_adaptivity=True, use_switch_estimator=True, recomputed=False): |
There was a problem hiding this comment.
Maybe you can generalise this function to arbitrary number of switches. Then you can import it from batter_2condensators_model.py.
| return sorted_list | ||
|
|
||
|
|
||
| def proof_assertions_description(description, use_adaptivity, use_switch_estimator): |
There was a problem hiding this comment.
maybe you could import this function from battery_2condensators.py and call it from here? Then you don't have to repeat the common requirements.
| plt_helper.plt.close(fig_acc) | ||
|
|
||
|
|
||
| def check_solution(stats, dt, problem, use_adaptivity, use_switch_estimator): |
There was a problem hiding this comment.
Maybe this function can be unified with the one from the model files? I am guessing one step size is the same? Then you could import this function from there and use it for testing.
|
I do not know what is going on here, having 30 files to merge.. I had a merge conflict as I merged from upstream/master. I will do some more changes, and then I will close this PR and request a new one.. |
4 participants
I adapted the switch estimator. Now it is a bit more flexible, at least for a new battery model. I changed the storage of switches, i.e., in the stats there is now only one type='switch', no longer type='switch1', type='switch2' and so on.. And the problem classes (both battery models) got a new function get_switching_info, where the switching conditions for each problem class are proved. I decided the switching conditions are problem specific, and the switch estimator is now more flexible for other problem classes.
The problem classes got t_switch as new attribute, many thanks to @brownbaerchen for the discussion! I also have to change the switching logics of the battery models, what I am not satisfying with, to be honest.. When we adapt the time step, due to find a switch, we restart. So we only adapt the time step. Ok. But we also need to specificly say the problem class, that something happen. In
https://github.com/lisawim/pySDC/blob/SE_new/pySDC/implementations/problem_classes/Battery.py#L87
you can find the switching logic. When we do not use the switching logic, then the we have:
if u[1] <= V_ref: do something, else: do something else
But in case of using the SE, we cannot use this evaluation no longer, because u[1] is always > V_ref. This is what we want to have. But in this case it is not enough to adapt the time step, we also need to do something in the problem class. One case for use the SE (if elf.t_switch is not None), and one case for no use of SE. The same for the Battery_2condensators.py. Here, the switches needs to be counted, otherwise I see no possibility to differ between the three different states when t_switch is overwriting in case of finding another switch.. I do not know if there is another possibility to write the evaluations, but perhaps this is okay at this moment. Nevertheless, I'm open for other (better) ideas.. Hope you nearly understand what I mean.