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

Classes to compute active/reactive power loads of the Coil Power Supply System #2967

Open
wants to merge 63 commits into
base: feature/powercycle
Choose a base branch
from

Conversation

tplobo
Copy link

@tplobo tplobo commented Jan 26, 2024

Linked Issues

Closes #3017.

Description

Interface Changes

Checklist

I confirm that I have completed the following checks:

  • Tests run locally and pass pytest tests --reactor
  • Code quality checks run locally and pass pre-commit run --from-ref develop --to-ref HEAD
  • Documentation built locally and checked sphinx-build -W documentation/source documentation/build

@tplobo tplobo requested review from a team as code owners January 26, 2024 15:17
@tplobo tplobo requested a review from a team as a code owner February 8, 2024 15:13
@je-cook je-cook force-pushed the tplobo/powercycle_coils branch 2 times, most recently from 57cc8b6 to d620c38 Compare February 8, 2024 15:47
Comment on lines 69 to 124
subclass_name = "CoilSupplyParameter"
single_value_types = (bool, int, float, list, tuple, np.ndarray)

@classmethod
def validate_parameter(cls, obj):
"""
Validate 'obj' to be of the same class as the object testing it.

Substitute method for 'isinstance', since it fails in this
implementation.
"""
return obj.__class__.__name__ == cls.__name__

@classmethod
def init_subclass(cls, argument: Any = None):
"""
Create a 'CoilSupplyParameter' subclass instance from argument.

If 'None' is given to instantiate the class, an empty instance
is created.
If an object of this class is given to instantiate the class, it
is returned as is.
If a 'dict' is given to instantiate the class, keys must match
class attributes and their values are distributed.
If a value of one of the 'single_value_types' classes is given
to instantiate the class, copy that value to all attributes.
"""
if argument is None:
return cls()
if cls.validate_parameter(argument):
return argument
if isinstance(argument, dict):
return cls(**argument)
if isinstance(argument, cls.single_value_types):
args = {}
all_fields = fields(cls)
for one_field in all_fields:
args[one_field.name] = argument
return cls(**args)
raise ValueError(
"A 'CoilSupplyParameter' instance must be initialized "
f"with 'None' for an empty instance, a '{cls.__name__}' "
"instance for no alteration, a 'dict' for a distributed "
"instantiation or any of the following types for a "
f"single-value instantiation: {cls.single_value_types}. "
f"Argument was '{type(argument)}' instead."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

None of this is needed please remove, its over complicating things again

Suggested change
subclass_name = "CoilSupplyParameter"
single_value_types = (bool, int, float, list, tuple, np.ndarray)
@classmethod
def validate_parameter(cls, obj):
"""
Validate 'obj' to be of the same class as the object testing it.
Substitute method for 'isinstance', since it fails in this
implementation.
"""
return obj.__class__.__name__ == cls.__name__
@classmethod
def init_subclass(cls, argument: Any = None):
"""
Create a 'CoilSupplyParameter' subclass instance from argument.
If 'None' is given to instantiate the class, an empty instance
is created.
If an object of this class is given to instantiate the class, it
is returned as is.
If a 'dict' is given to instantiate the class, keys must match
class attributes and their values are distributed.
If a value of one of the 'single_value_types' classes is given
to instantiate the class, copy that value to all attributes.
"""
if argument is None:
return cls()
if cls.validate_parameter(argument):
return argument
if isinstance(argument, dict):
return cls(**argument)
if isinstance(argument, cls.single_value_types):
args = {}
all_fields = fields(cls)
for one_field in all_fields:
args[one_field.name] = argument
return cls(**args)
raise ValueError(
"A 'CoilSupplyParameter' instance must be initialized "
f"with 'None' for an empty instance, a '{cls.__name__}' "
"instance for no alteration, a 'dict' for a distributed "
"instantiation or any of the following types for a "
f"single-value instantiation: {cls.single_value_types}. "
f"Argument was '{type(argument)}' instead."
)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @je-cook,

this one is a bit more complicated than that... I have made a "dataclass factory" to create dataclasses to store vectors for a CoilSupplySystem object. This ensures the dataclass has one vector for each coil defined in the CoilSupplySystem. These methods are not so much for validation, but rather automatized creation of these containers. I call the related methods "validation" but they are used to instantiate the container objects independent of the input so maybe the term is not the most appropriate.

In short, the workflow is as follows:

  1. a CoilSupplyInputs object is initialized, with a certain list of coils in config.coil_names
  2. this automatically specifies what fields the container dataclass must have, so a CoilSupplyParameter class is defined as child of CoilSupplyParameterABC, and stored in the parameter attribute of CoilSupplyInputs
  3. a CoilSupplySystem object is initialized with the CoilSupplyInputs object, so all methods that need to create a container can do it easily, either empty (full of None values) or passing an object to validate_parameter... dictionaries are distributed in the dataclass, other objects are just copied.

Perhaps that's not the best way to do this, but this was meant to be discussed with you and @oliverfunk

Let me know what you think,

Comment on lines 148 to 162
if self.validate_parameter(other):
other_value = getattr(other, one_field.name)
Copy link
Contributor

Choose a reason for hiding this comment

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

stop validating

Suggested change
if self.validate_parameter(other):
other_value = getattr(other, one_field.name)

@tplobo tplobo force-pushed the tplobo/powercycle_coils branch 2 times, most recently from 5905d59 to 8a699ab Compare April 16, 2024 16:52
@tplobo tplobo changed the title Classes to compute active/reactive power loads by the Coil Power Supply System Classes to compute active/reactive power loads of the Coil Power Supply System Apr 22, 2024
Copy link

sonarcloud bot commented Apr 22, 2024

Quality Gate Passed Quality Gate passed

Issues
23 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.0% Duplication on New Code

See analysis details on SonarCloud

@je-cook je-cook force-pushed the feature/powercycle branch 4 times, most recently from 9ae94a0 to b43893d Compare May 6, 2024 21:31
@je-cook je-cook force-pushed the feature/powercycle branch 3 times, most recently from 4b7d1c4 to fd885c4 Compare June 4, 2024 12:56
@tplobo
Copy link
Author

tplobo commented Jun 5, 2024

@je-cook, I assume this one should also be rebased on feature/powercycle, instead of develop... what is the argument for --onto in this case? I don't understand what should be the equivalent one for powercycle_coils

@tplobo tplobo requested a review from je-cook June 5, 2024 10:17
@tplobo tplobo self-assigned this Jun 10, 2024
Copy link

sonarcloud bot commented Jun 18, 2024

Quality Gate Passed Quality Gate passed

Issues
31 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.9% Duplication on New Code

See analysis details on SonarCloud

@je-cook
Copy link
Contributor

je-cook commented Jul 8, 2024

@tplobo can this be rebase on top of feature/powercycle please? It a bit impossible to review at the moment

tplobo added 22 commits July 23, 2024 11:41
…rameter` and `verbose` argument to `compute_wallplug_loads`
Copy link

sonarcloud bot commented Jul 23, 2024

@tplobo
Copy link
Author

tplobo commented Jul 23, 2024

once you've pulled the most recent commit on feature/powercycle this is command. You branched at 1db075f git rebase --onto feature/powercycle 1db075f6

EDIT looks like there is a modified commit compared to the feature branch so 2 things before the above:

git rebase --interactive HEAD~59

edit line 2 save and exit

before pick e929c15c Powercycle layout base (#2873) after pick 1db075f6 Powercycle layout base (#2873)

@je-cook, I have followed your instructions, but as I mentioned I had to make some corrections while interactively merging due to some conflicts; I've run my tests but you might encounter a bug or another. I have also added the IDM files used in verification as ignored files in a local .gitignore; let me know if we should change approach that to something else. Also, is it necessary to run a filter-repo to remove the previous files, or can we leave it as is?
Finally, to complete the tests I have also had to modify match_domains because one of the last numpy updates broke it. Thanks for the help and let me know if you need any support during review.

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

Successfully merging this pull request may close these issues.

2 participants