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

Investigate whether Damages can work with only one type of 'graph'. #647

Closed
Carsopre opened this issue Dec 10, 2024 · 1 comment
Closed
Assignees
Labels
architecture refactoring Architectural changes to be addressed by the GAP team enhancement New feature or request

Comments

@Carsopre
Copy link
Collaborator

Carsopre commented Dec 10, 2024

Kind of request

Changing existing functionality

Enhancement Description

Currently we initialized damages with the AnalysisInputWrapper as well as the MultiGraph from analysis_config.graph_files.base_graph_hazard.get_graph(). This implies that within the class Damages we have at the same time the "complex" and the "simplified" network / graph.

We would like Damages to only work with either of them (if possible), therefore removing the need of the later introduced Damages.reference_base_graph_hazard: MultiGraph, otherwise we should modify the (data)class AnalysisInputWrapper so that it considers both "simplified' and "complex" network(s) (rename the existing ones and add the new ones).

Ultimately we want to understand whether both simplified and complex graphs are needed, and if so then explicitly write it.

Use case

Constructors of analysis should only consume the AnalysisInputWrapper without needing extra arguments.

Additional Context

This is an improvement of the existing code. This kind of issues are helping us understand how to better shape the creation and execution of all the different analyses ( AnalysisProtocol).

This is an investigation issue, in case of inconclusive results, we do not need to merge anything.

@Carsopre Carsopre added architecture refactoring Architectural changes to be addressed by the GAP team enhancement New feature or request labels Dec 10, 2024
@Carsopre Carsopre added this to the Sprint 2024 Q4.4 milestone Dec 10, 2024
@sahand-asgarpour
Copy link
Contributor

sahand-asgarpour commented Dec 10, 2024

@Carsopre

Results of the investigation

  • Difference between the complex and simplified graphs/networks=>
    • analysis_config.graph_files.base_network_hazard is the GeoDataFrame of the complex graph => complex graph edges object might have multiple segments between intersection to intersection

    • analysis_config.graph_files.base_graph_hazard is the simplified NetworkX Graph object of the base_network_hazard => simplified graph edges object considers the whole collection of segments between intersection to intersection as ONE LINK

  1. If the user is ONLY interested in running the damage function => the user wants to know where damages occur => complex graph gives more detailed insights => base_network_hazard suffices => Damages.graph_file_hazard should be used. => this already happens in Damages.execute()

  2. If the user wants to run the Losses and Damages and aggregate the results for BENEFITS of ADAPTATION => Since Losses use simplified graph => damages on the simplified graph should be known => Damages should get link-based results => link_based results means results for the simplified graph object (which considers the whole collection of segments between intersection to intersection as ONE LINK) => this function currently uses analysis_config.graph_files.base_graph_hazard (Damages.reference_base_graph_hazard).

Actions and decisions

  • Do we want to have a rigid or flexible way of running damages:

    • Flexible: get link_based results when needed (see point 2) and the standard way of running damages remain using base_network_hazard (point 1)

      • Benefits: Runs more efficient
      • Downside: complexities in the running the analyses => Quick fix: get result link based when we run AdaptiationOptionAnalysis
    • Rigid (current approach): always getting link-based damages using _get_result_link_based() => we need to pass base_graph_hazard to the Damages as well

      • Benefits: No complexity in running analyses
      • Downside: inefficiency. + we might need to resolve the inefficiency of the _get_result_link_based() method.
  • Here, getting link-based damages result can be done in two ways. It might be important to understand which of the following is more efficient.
    - Current method: _get_result_link_based() => too heavy for a network of 1.5 mil edges
    - Adapt Damages.execute to accept MultiGraph (analysis_config.graph_files.base_graph_hazard) => run damages directly on the simplified graph rather than constructing it (as done in _get_result_link_based())

Quick-fix in this PR

  • A flexible version is proposed, where
    • in the Damages, we work only with base_network_hazard
    • In the AdaptationOptionCollection execute we work with base_graph_hazard and _get_result_link_based()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture refactoring Architectural changes to be addressed by the GAP team enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants