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

refactor: create class variables for supported models and engines #517

Conversation

goccert25
Copy link
Contributor

Pull Request doc

Purpose

What is the larger goal of this change?
Create a single place where supported models and engines are listed for the Grid class. This will allow the download manager project to easily use those supported models and engines as choices for CLI parameters

What the code is doing

How is the purpose executed?
Adding class variables to Grid

Testing

How did you test this change (unit/functional testing, manual testing, etc.)?
unit tests

Where to look

N/A

Usage Example/Visuals

N/A

Time estimate

How long will it take for reviewers and observers to understand this code change?
1 minute

supported_engines = {"REISE", "REISE.jl"}
if engine not in supported_engines:
raise ValueError(f"Engine must be one of {','.join(supported_engines)}")
if source not in self.SUPPORTED_MODELS:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe source can also be a path to a .mat file which contains a modified grid. See L#47 where this is handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jon-hagg updated the input validation to account for this. I think long term though the source parameter should be refactored to be an object that's passed into this class. In other words, this logic:

        if cached is not None:
            data = cached
        elif source == "usa_tamu":
            data = TAMU(interconnect)
        elif os.path.splitext(source)[1] == ".mat":
            if engine == "REISE":
                data = FromREISE(source)
            elif engine == "REISE.jl":
                data = FromREISEjl(source)

should exist outside of the Grid class. This makes the code more maintainable by using dependency injection. As the code base grows, adding more source/engine options and tests to this class is going to become more of a PITA. Thoughts on adding a github issue/jira ticket/asana task (not sure how action items are tracked 😅 ) for refactoring this Grid class?

Copy link
Contributor

Choose a reason for hiding this comment

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

A github issue is probably best. This idea doesn't quite fit into our definition of a 'feature', but I think the feature request issue template is probably the best to use for a refactor like this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We also need to factor out a lot of code from the TAMU class since other grid models will have the same input files (CSV) with the same columns and header.

Copy link
Contributor

Choose a reason for hiding this comment

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

Part of that work was done in my branch daniel/read_hifld_grid, which is behind develop by a bit, but most of the relevant commits should be cherry-pickable without any merge conflicts.

@goccert25 goccert25 force-pushed the goccert25/add_supported_models branch from e67fa82 to eb80499 Compare July 7, 2021 19:20
Copy link
Collaborator

@jenhagg jenhagg left a comment

Choose a reason for hiding this comment

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

LGTM

@jenhagg jenhagg merged commit dabd23b into Breakthrough-Energy:develop Jul 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants