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: run prototype with command line tool #36

Merged
merged 36 commits into from
Aug 28, 2024
Merged

Conversation

ChouaneLouis
Copy link
Contributor

No description provided.

@sylvlecl sylvlecl changed the title Feature/executable Feature: run prototype with command line tool May 13, 2024
@sylvlecl sylvlecl changed the base branch from main to add-yaml-test May 13, 2024 12:40
@sylvlecl sylvlecl changed the base branch from add-yaml-test to main May 13, 2024 12:41
return resolve_components_and_cnx(parse_yaml_components(comp), model)


if __name__ == "__main__":
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, I suggest to put all that is bellow here in a "main" function:

Suggested change
if __name__ == "__main__":
if __name__ == "__main__":
main()

enable the possibility to use the command with only the parameter --study
fix the fact that the preloaded_libraries parameter in the resolve_library method didn't keep the preloaded models in the returned library
database = None

if args.study:
if args.models or args.component or args.timeseries:
Copy link
Contributor

@ianmnz ianmnz May 22, 2024

Choose a reason for hiding this comment

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

I think there's a way in ParseArgs to create exclusive groups, i.e., sets of arguments that can't be passed together so you would automatically check for this, but maybe it won't be enough for your needs here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is but you can't make an argument excludes with other arguments which are not mutually exclusive

ChouaneLouis and others added 2 commits May 23, 2024 11:20
the importing order is an depth-first travelsal of the tree going from every library to the root or an already imported library
the algo :
 - checks if there is cycles and raise error if so
 - can import models form 2 separated tree
 - doesnt check if ports are defined twice

no test available
the code isnt commented
Copy link
Member

@sylvlecl sylvlecl left a comment

Choose a reason for hiding this comment

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

A few suggestions:

  • adding a "console script" entry point
  • moving some new methods to more appropriate places in inner modules

@@ -93,6 +93,7 @@ class Config:

class InputLibrary(BaseModel):
id: str
dependence: Optional[str] = None
Copy link
Member

Choose a reason for hiding this comment

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

We should allow a list here to import multiple libraries --> "dependencies"

src/andromede/main/main.py Show resolved Hide resolved
if import_stack[-1].dependence in did:
lib = resolve_library(import_stack[-1], [output_lib])

output_lib.models.update(lib.models)
Copy link
Member

Choose a reason for hiding this comment

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

we can add a check here that there is no name conflict between models

lib = resolve_library(import_stack[-1], [output_lib])

output_lib.models.update(lib.models)
output_lib.port_types.update(lib.port_types)
Copy link
Member

Choose a reason for hiding this comment

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

we can add a check here that there is no name conflict between port types

yaml_libraries[yaml_lib.id] = yaml_lib

todo = list(yaml_libraries.values())
did = list()
Copy link
Member

Choose a reason for hiding this comment

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

Note that it's better to use a set for this kind of use: a search in a set is more efficient than in a list (to be honest here it does not really matter in practice because it will always be small).

parser.error(
"--study flag can't be use with --models, --component and --timeseries"
)
components_path = args.study / "input" / "components" / "components.yml"
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this logic (of parsing all data from one directory) to andromede.study.parsing

print("scenario ", scenario)
print("status : ", status)

print("avarage final cost : ", problem.solver.Objective().Value())
Copy link
Member

Choose a reason for hiding this comment

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

typo "average"

def main() -> None:
parser = argparse.ArgumentParser()
parser.add_argument(
"--study", type=Path, help="path to the root dirertory of the study"
Copy link
Member

Choose a reason for hiding this comment

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

typo directory

"--component", type=Path, help="path to the component file, *.yml"
)
parser.add_argument(
"--timeseries", type=Path, help="path to the timeseries dirertory"
Copy link
Member

Choose a reason for hiding this comment

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

typo directory

@ianmnz ianmnz requested a review from sylvlecl August 28, 2024 13:56
@ianmnz ianmnz merged commit 52e01dd into main Aug 28, 2024
1 check passed
@ianmnz ianmnz deleted the feature/executable branch August 28, 2024 14:25
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