-
Notifications
You must be signed in to change notification settings - Fork 111
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
PR #530 follow up #533
PR #530 follow up #533
Conversation
self.from_csv = system.options.get("from_csv") | ||
if self.from_csv is not None: | ||
self.data_csv = self._load_csv(self.from_csv) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_load_csv()
after init()
, to ensure smooth init when from_csv is given in TDS.run()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the issue with the previous order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is when CSV is given at "andes.load(from_csv='xx.csv')", TDS.init()
run into error for the incompatible data size. It is because _load_csv()
is called after TDS.init()
in TDS.run()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can forget this as the introduced helper function TDS._csv_output()
addressed it.
andes/routines/tds.py
Outdated
xyidx = system.Output.xidx + [yidx+system.dae.n for yidx in system.Output.yidx] | ||
t = data[:, 0] | ||
_xy[:, xyidx] = data[:, 1:] | ||
data = np.hstack((t[:, np.newaxis], _xy)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
padding data with zeros in _load_csv()
to ensure only one-time execution.
self.from_csv = system.options.get("from_csv") | ||
if self.from_csv is not None: | ||
self.data_csv = self._load_csv(self.from_csv) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the issue with the previous order?
andes/routines/tds.py
Outdated
logger.warning("CSV data contains fewer variables than required.") | ||
logger.warning("Check if the CSV data file is generated with selected output.") | ||
else: | ||
# reconstruct data with padding zeros | ||
_xy = np.zeros((data.shape[0], system.dae.n + system.dae.m)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might work, but it seems to waste a lot of memory.
If the system has 100 total variables, memory for all of them will be preallocated for each step. If the Output only has three variables, then only three out of 100 will be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just store the indices? Do a one-time read-in of the CSV data. At each step, index into dae.x
and dae.y
with the stored indices and overwrite them with the CSV data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improved by adding a helper function to fit the recorded data in TDS._csv_step()
.
andes/routines/tds.py
Outdated
@@ -1108,3 +1090,22 @@ def check_criteria(self): | |||
res = deltadelta(self.system.dae.x[self.system.SynGen.delta_addr], | |||
self.config.ddelta_limit) | |||
return res | |||
|
|||
def _csv_output(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming this for clarity. _csv_output
reads like outputting to a CSV file.
Consider _csv_data_to_dae
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add a test case under tests
by the way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changed lines are covered by following tests. Do you mean a test on the helper function itself?
Lines 78 to 129 in 7f56cc9
def test_from_csv(self): | |
""" | |
Test from_csv when loading selected output from csv file. | |
""" | |
case = andes.get_case("5bus/pjm5bus.json") | |
ss = andes.load(case, | |
no_output=True, | |
setup=False, | |
default_config=True) | |
ss.add("Output", {"model": "Bus", "varname": "v"}) | |
ss.setup() | |
ss.PFlow.run() | |
ss.TDS.config.tf = 0.1 | |
ss.TDS.run() | |
ss.TDS.load_plotter() | |
ss.TDS.plt.export_csv("pjm5bus_selec_out.csv") | |
# Test assign CSV in TDS.run() | |
ss2 = andes.load(case, | |
no_output=True, | |
setup=False, | |
default_config=True) | |
ss2.add("Output", {"model": "Bus", "varname": "v"}) | |
ss2.setup() | |
ss2.PFlow.run() | |
ss2.TDS.run(from_csv="pjm5bus_selec_out.csv") | |
self.assertTrue(ss2.TDS.converged) | |
# Test assign CSV in andes.load() | |
ss3 = andes.load(case, | |
no_output=True, | |
setup=False, | |
default_config=True, | |
from_csv="pjm5bus_selec_out.csv") | |
ss3.add("Output", {"model": "Bus", "varname": "v"}) | |
ss3.setup() | |
ss3.PFlow.run() | |
ss3.TDS.run() | |
self.assertTrue(ss3.TDS.converged) | |
os.remove("pjm5bus_selec_out.csv") |
Oh the test looks good!
Get Outlook for iOS<https://aka.ms/o0ukef>
________________________________
From: Jinning Wang ***@***.***>
Sent: Tuesday, April 16, 2024 4:36:11 PM
To: CURENT/andes ***@***.***>
Cc: Hantao Cui ***@***.***>; Review requested ***@***.***>
Subject: Re: [CURENT/andes] PR #530 follow up (PR #533)
@jinningwang commented on this pull request.
________________________________
In andes/routines/tds.py<#533 (comment)>:
@@ -1108,3 +1090,22 @@ def check_criteria(self):
res = deltadelta(self.system.dae.x[self.system.SynGen.delta_addr],
self.config.ddelta_limit)
return res
+
+ def _csv_output(self):
The changed lines are covered by following tests. Do you mean a test on the helper function itself? https://github.com/CURENT/andes/blob/7f56cc9ea4c16d9d58f5b440984f7ee2e6fd1018/tests/test_output.py#L78-L129
—
Reply to this email directly, view it on GitHub<#533 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABSNZA2KNQFKU5PRWU67W3DY5WKUXAVCNFSM6AAAAABGGFCRLSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBUGYZTSMRVGY>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
Jinning, thanks a lot! |
_csv_data_to_dae()
Related to #526