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

Feature/time series load update and sgen update #184

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Laurynas-Jagutis
Copy link
Member

No description provided.

@Laurynas-Jagutis Laurynas-Jagutis added the feature New feature or request label May 8, 2023
Copy link
Member

@mgovers mgovers left a comment

Choose a reason for hiding this comment

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

i didn't dig too deep into the code yet but here are some simple remarks to get things started

Comment on lines 84 to 89
# Set pandas data
self.pp_input_data = data
self._create_input_data()
elif data_type == "update":
self.pp_update_data = data
self._update_input_data()
Copy link
Member

Choose a reason for hiding this comment

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

since this function is called _parse_data, this may not be the right place to actually set the data. could be out of scope though

Alternatively, maybe it's a good moment to split the (minor) chunks of differences between input_data, output_data and update_data into separate classes

@@ -65,18 +72,21 @@ def _parse_data(self, data: PandaPowerData, data_type: str, extra_info: Optional
Returns:
Converted power-grid-model data
Copy link
Member

Choose a reason for hiding this comment

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

right now, it always returns self.pgm_input_data. I think it should return self.pgm_update_data when the data_type == "update", right?

Comment on lines 76 to 80
self.pgm_input_data = {}
self.idx_lookup = {}
self.next_idx = 0

# Set pandas data
self.pp_input_data = data
self.pgm_update_data = {}
Copy link
Member

Choose a reason for hiding this comment

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

nitpick but please keep similar fields close together for readability, e.g.:

        self.pgm_input_data = {}
        self.pgm_update_data = {}
        self.idx_lookup = {}
        self.next_idx = 0

@@ -247,7 +257,7 @@ def _extra_info_to_pgm_input_data(self, extra_info: ExtraInfo): # pylint: disab
nan = np.iinfo(dtype).min
all_other_cols = ["i_n"]
for component, data in self.pgm_output_data.items():
input_cols = power_grid_meta_data["input"][component].dtype.names
input_cols = power_grid_meta_data["input"][component]["dtype"].names
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 a bug? or a deprecated feature of structured arrays? the original one feels more correct but i could be wrong

Comment on lines 2068 to 2069
const_i_multiplier = self._get_pp_attr("load", "const_i_percent", 0) * scaling * (1e-2 * 1e6)
const_z_multiplier = self._get_pp_attr("load", "const_z_percent", 0) * scaling * (1e-2 * 1e6)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const_i_multiplier = self._get_pp_attr("load", "const_i_percent", 0) * scaling * (1e-2 * 1e6)
const_z_multiplier = self._get_pp_attr("load", "const_z_percent", 0) * scaling * (1e-2 * 1e6)
const_i_multiplier = self._get_pp_attr("load", "const_i_percent", 0) * scaling * 1e4
const_z_multiplier = self._get_pp_attr("load", "const_z_percent", 0) * scaling * 1e4

or split the coefficients, e.g. (but please clean up a bit by reordening):

Suggested change
const_i_multiplier = self._get_pp_attr("load", "const_i_percent", 0) * scaling * (1e-2 * 1e6)
const_z_multiplier = self._get_pp_attr("load", "const_z_percent", 0) * scaling * (1e-2 * 1e6)
percent = 1e-2
unit_magnitude = 1e6
scaling *= unit_magnitude
const_i_multiplier = self._get_pp_attr("load", "const_i_percent", 0) * percent * scaling
const_z_multiplier = self._get_pp_attr("load", "const_z_percent", 0) * percent * scaling

Comment on lines 2084 to 2085
# Convert it to a list, debugger was complaining
pp_load_ids = list(pp_load_ids)
Copy link
Member

Choose a reason for hiding this comment

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

why does it need to be a set in the first place? why not start with a list? or, if it should be a set, there's probably a problem somewhere down the line 😄

For instance, i don't know if _get_timeseries_load_ids wants a list as input. If so, either start with a list or convert to list there.

If you really need the uniqueness feature of set, as well as use the list output in multiple places, then please update the comment instead


self.pgm_update_data["sym_load"] = pgm_load_profile

#
Copy link
Member

Choose a reason for hiding this comment

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

todo: remove before merge (you can always re-add later)

Comment on lines 2233 to 2234
# pylint: disable-msg=too-many-locals
def _pp_update_sgens(self): # pragma: no cover
Copy link
Member

Choose a reason for hiding this comment

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

todo: probably a good idea to split in separate functions if it complains about this

(note: i understand why it's still in there, so no hurry. i just add this comment for reference)

@@ -134,6 +139,7 @@ def test_parse_data__extra_info(
fill_pp_extra_info_mock.assert_called_once_with(extra_info=extra_info)


@pytest.mark.xfail()
Copy link
Member

Choose a reason for hiding this comment

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

todo: i'm guessing you just haven't updated the test yet, which is fine, hence the todo 😄

Signed-off-by: Laurynas Jagutis <[email protected]>
@sonarcloud
Copy link

sonarcloud bot commented May 15, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

65.0% 65.0% Coverage
0.0% 0.0% Duplication

@mgovers
Copy link
Member

mgovers commented Jul 25, 2023

@TonyXiang8787 @Laurynas-Jagutis is this PR still relevant?

@TonyXiang8787
Copy link
Member

@mgovers we have decided to on hold this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants