Clean-up for PinTSimE + Update for SwitchEstimator (Preparation for PR for paper stuff) #362
Conversation
…erent number of coll. nodes + updated hardcoded solutions
…ts for both DAE models from paper
|
Please merge |
There was a problem hiding this comment.
This is a pretty big PR, so I did not go through everything in detail. Particularly, I did not look at plotting at all.
At this point, I am not 100% sure what the functionality is that you are after, so I did not go into detail to understand what the functions are doing. I trust you that you made sure the functionality is what you want and that the tests are meaningful.
I think you greatly improved on readability in many places. Good job!
I do have some minor comments, concerns, though. The most important one being the filtering of recomputed values, I think.
| if V_ref is None and ncapacitors == 1: | ||
| V_ref = np.array([1.0]) | ||
| else: | ||
| assert (alpha > V_ref[k] for k in range(n)), 'Please set "alpha" greater than values of "V_ref"' |
There was a problem hiding this comment.
To be honest, I don't totally know what happens here. I usually use all or any for asserting things like this. Since black did not complain, this must be valid. But please be sure that it does what you want it to do and consider adding all to improve readability by being more explicit.
Also, this line should come after the check if V_ref is an array at all to avoid confusing error messages.
| if C is None: | ||
| C = np.array([1.0]) | ||
| else: | ||
| assert type(C) == np.ndarray, '"C" needs to be an np.ndarray' | ||
| assert np.shape(C)[0] == n, 'Number of capacitance values needs to be equal to number of condensators' |
There was a problem hiding this comment.
typo: capacitors instead of condensators
| assert alpha > V_ref[0], 'Please set "alpha" greater than values of "V_ref"' | ||
| assert type(V_ref) == np.ndarray, '"V_ref" needs to be an np.ndarray' | ||
| assert np.shape(V_ref)[0] == n, 'Number of reference values needs to be equal to number of condensators' | ||
| assert V_ref[0] > 0, 'Please "V_ref" greater than 0' |
There was a problem hiding this comment.
Why do you check these things here? You inherit from the battery class, which I believe fixes the number of capacitors to 1. And this, in turn, inherits from battery_n_capticitors, which performs these tests anyways, making them redundant here.
I suggest removing these tests here and, if you want, adding an __init__ to battery that only makes sure that n_capacitors == 1 and calls the super init.
The only thing you need here is the variables for counting Newton iterations, which you should ideally replace with the WorkCounters for consistent interfaces among problems.
There was a problem hiding this comment.
Yes, you are right!
WorkCounters everywhere! I'll add that!
| h = 2 * y - 100 | ||
| f = self.dtype_f(self.init) | ||
|
|
||
| f_before_event = ( |
There was a problem hiding this comment.
Why do you compute both and then check which to assign to f[:] instead of computing while assigning? It probably makes little difference for speed, but aids readability in my opinion.
There was a problem hiding this comment.
I also add that for readability, but I agree with you. Only one of these f is necessary, hence no need to compute both at every time!
| Exact solution. | ||
| """ | ||
|
|
||
| assert t >= 1, 'ERROR: u_exact only valid for t>=1' |
There was a problem hiding this comment.
How do you get the initial conditions? Do you have to start at t=1? Also, I suggest replacing valid with available.
There was a problem hiding this comment.
Yes, it's possible to compute the initial condition starting at t=1. but also at a later time. I'll adapt that!
| # dangerous! in case of nonmoving state function after event, event time could be distorted! | ||
| t_switch = t_interp[0] | ||
| boundary = 'left' | ||
| self.log(f"Is already close enough to the {boundary} end point!", S) |
There was a problem hiding this comment.
Maybe this message could be a bit more elaborate.
| elif abs(state_function[-1]) <= self.params.tol_zero: | ||
| boundary = 'right' | ||
| t_switch = t_interp[-1] | ||
| else: |
There was a problem hiding this comment.
Can this ever be entered? The above statement is if either is close to zero you go here and this you enter only if neither is close to zero...?
|
|
||
|
|
||
| @pytest.mark.base | ||
| def test_DiscontinuousTestDAE_SDC(): |
There was a problem hiding this comment.
you can make M and argument of this function and add a decorator @pytest.mark.parametrize('M', [2, 3, 4, 5]) to separate this into different tests. I find this a bit nicer because the error handling will be more precise. Maybe the tests only failed for 3 nodes, for instance.
| 5: 0.0101, | ||
| } | ||
|
|
||
| for M in [2, 3, 4, 5]: |
There was a problem hiding this comment.
You can again make this an argument of the function if you want.
| @@ -357,3 +391,47 @@ def newton(x0, p, fprime, newton_tol, newton_maxiter): | |||
| print(msg) | |||
|
|
|||
| return root | |||
|
|
|||
|
|
|||
| class LagrangeInterpolation(object): | |||
There was a problem hiding this comment.
You could consider using Thibaut's interpolation stuff, which is well tested. I am not sure how the two things compare on speed. With Thibaut's stuff, you have to generate a new interpolation matrix every time you want to interpolate to a new time and multiply this with the solution vector. But this might be faster than generating the Lagrange polynomials from scratch every time. Regardless, building on well tested stuff is always nice.
|
Yeah, no, I'd rather not introduce another dependency, unless there is a strong need for this. And if we introduce another dependency, this has to work (currently) with versions 3.7-3.10, later also 3.11 and 3.12. |
What do you mean? You don't want another dependency or not another environment? The tests are currently failing because of the missing dependency and I have no idea how much work it would be to replace this by hand or by scipy or whatever. |
|
Ok, and how should I deal with the |
|
How important is this dependency? Is this a "nice to have" or a "must have, no other suitable option"? Introducing another dependency file ( |
|
The |
|
There is a workaround: I can also implement the WSCC9bus network hard-codded in this problem file, so then the dependency can be removed. |
|
Oh, yes please!! I was about to write a lengthy description what we need to do if we want to keep pypower in.. |
|
Hm, I know that we also tried another packages for computing the initial conditions. But in the end, these are only fixed values coming from the |
|
Yes, what I wrote. |
|
If you manage to do this without any other (new) dependency, I'm happy! |
|
You may want to consider splitting this example from the PR, though. This is a big PR and maybe the big power grid examples "deserves" its own PR. |
|
So, this means closing this PR and request new (at least two) ones? |
|
You could (I'm just suggesting) remove the big example here and add it to a new one. |
You don't have to close this one. Just add a new PR with only the problem and then you can decide if you want to remove the problem from this PR (which you can do without closing) or if you want to wait until we merge the other PR and then you can merge master here again. I like to do and then for each file that you need: Then try if the tests work, maybe adapt a few things, commit, push, PR. You can also use this as a stash for later, though. What Robert suggested may be simpler. |
|
I decided not to move the big power system example to a separate PR, but the whole paper stuff, and the clean-up here (together with an updated version of the switch estimator is then a preparation to this PR). Before merging I'll also have a look at the Lagrange Interpolation in the switch estimator at some time.. |
lisawim
changed the title
|
Do you want me to read through the code again? Anything new besides the things you mentioned and I complained about earlier? |
|
Please merge from |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #362 +/- ##
==========================================
- Coverage 73.26% 72.55% -0.71%
==========================================
Files 270 269 -1
Lines 23097 22330 -767
==========================================
- Hits 16922 16202 -720
+ Misses 6175 6128 -47
☔ View full report in Codecov by Sentry. |
|
Anyone wants to have another look here? Are there still open todos for this PR? |
|
I also want to have a look at the Lagrange interpolation and probably use @tlunet's stuff. But I already used the current version of the Switch Estimator (not including @tlunet's stuff yet) for producing the results for the paper, and I didn't test yet if this would lead to the same results. In order to reproduce the plots from the paper I would feel more comfortable if this PR would be merged and then I'll deal with the Lagrange interpolation from @tlunet in a PR after merge the paper stuff. Is that fine for you? And I also don't know if @brownbaerchen has further compliants. I changed all requested things from him. From my side he shouldn't have another look at that. |
|
LGTM Thibaut's stuff should give the same result. Both is Lagrange interpolation, just with a different interface. But from my point of view it's fine to merge this now. |
* Added README for paper stuff * Lagrange interpolation for SwitchEstimator * Clean-up for project, i.e., merge stuff for all battery models together * add WSCC9Bus DAE test case * PascalCase for functions + possibility of computing solution for different number of coll. nodes + updated hardcoded solutions * Added stuff for paper with new problem DTestDAE and corresponding tests for both DAE models from paper * Updated tests with hardcoded solutions for PinTSimE * Clean-up also for piline+buck_converter, and update for playground * Minor changes + black.. * Update README for paper + add pypower to environment-base.yml * ? * Removed stuff for paper (will moved to another PR) * Requested changes * add WSCC09 test case with cleaned dependency * Added test for WSCC9 * fix WSCC9 case, minor error fix * Added discontinuous test DAE + test and black for WSCC9 * Minor changes for DiscontinuousTestDAE * Added folder for paper stuff * Removed generated plots * Added commit to README to be used for generating paper stuff * Requested changes * WIP: add threeInverterSystem * Removed class again (not part of this PR) * Shortened test for WSCC9 bus test case * Oh black.. how I could forget you..? * Renamed test * One more test for WSCC9 --------- Co-authored-by: Junjie Zhang <junjie.zhang1@rwth-aachen.de>
Wui, finished!
For the submitted paper I created a folder in the PinTSimE project folder containing
The paper contains two new discontinuous DAE models: a test DAE
DiscontinuousTestDAEcontaining an exact solution and an exact event time, and the more realistic one,WSCC9BusSystema power system case where @junjie-zh and I spend several weeks to implement this one inpySDC. The latter one contains 57 equations.. Thus a solve is very expensive (also in view of computing time). Executing the scriptpaper_plots.pytakes approximately nine hours (ugh!) which is the reason why I disabled it in the script.. But I write tests for both DAE classes to ensure that everything goes right.SwitchEstimatorgot one more update: I added Lagrange interpolation to interpolate the dynamics of the state function. This required complete rewriting of the tests already contained.Since I'm not satisfied with the total mess in the PinTSimE project especially in the files to the battery models, I rewrite almost everything to have more clarification of what and how is done there. But the most important thing: readability has improved significantly. At least I hope so.. For the numerical solution a dictionary
u_numis created which provides all the important stuff to generate beautiful plots. There is also the possibility to consider different number of collocation nodes. The old plots for the battery model does only make sense to plot stuff for one coll. node, otherwise they will be "overloaded".. But such a dictionary makes it easier to implement plots containing results for more than one collocation node, which is for instance already done in the plots for the paper.Next thing.. The Python script for generating the paper plots
paper_plots.pyis not yet consistent with the stuff I did for the battery models (I mean the script has still another structure to compute the dictionary for the numerical solution, bla bla ..) But I don't want to change it now. I left this for the (far) future..Then I used the possibility to also clean up the files for
pilineandbuck_converterand updated their hardcoded solutions. I updated the playgroundEnergyGridsas well and removed the log data files there, because they are not needed anymore.Since
pypoweris needed for the problem classWSCC9BusSystem, I guessenvironment-base.ymlhas to be updated which I did.Hm, I hope I forgot nothing..
I know this is a big PR and I really apologize for that, but I would really appreciate it if you could take a look. Thank you so much for your time in advance! I guess you don't have to take a look at the PinTSimE stuff, and the playground, but rather for the stuff for paper.