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

Parallel years cleaning : few removal of "numspace" #2128

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

guilpier-code
Copy link
Contributor

@guilpier-code guilpier-code commented May 29, 2024

See ticket #1751

For now, just a first analyze of how we could do to isolate years (from the simulation and what should not be inside a year).
We take advantage of this to make some removals of numSpace.

Comments :

  • year job and simulation are really intertwined, this is a first and important reason why a year are not isolated.
    For instance, when running a year of simulation, we call a function of the simulation object, not of the year object.
  • although what we first thought, each year job creates a fresh new weekly optimization solver object. This object solves every week problem, using the previous week's optimization solution as a base. Before creating a solver object, a year first destroys the previous year's solver (as well as the start base it holds).
  • I've heard that CI tests for parallel (currently passed at night run) don't generate the same output results as the same tests run in sequential. We assumed that it was because of the bases. Indeed, if the same simulation year is run starting from 2 different bases, it can lead to different simulation results. But given the previous point, that's not true : the simulation results should be identical whether we run a simulation sequentially or not. We may have broken something in the parallelism by not testing the it on CI.

Goals :
We want to isolate year jobs to make cleaner parallelism.
Each year job should be designed so that it can be run in a thread independently from any other year job.
In particular, a year job should hold its own memory spaces, and not shared memory as done currently.

@guilpier-code guilpier-code marked this pull request as draft May 29, 2024 17:13
@guilpier-code guilpier-code changed the title Parallel year cleaning : few removal of "numspace" Parallel years cleaning : few removal of "numspace" May 29, 2024
@pull-request-size pull-request-size bot added size/L and removed size/M labels May 30, 2024
@@ -113,65 +113,6 @@ static void RecalculDesEchangesMoyens(Data::Study& study,
}
}

void PrepareDataFromClustersInMustrunMode(Data::Study& study,
Copy link
Contributor Author

@guilpier-code guilpier-code May 30, 2024

Choose a reason for hiding this comment

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

This function (PrepareDataFromClustersInMustrunMode) has many drawbacks :

  • It's too long
  • It's supposed to offer a treatment common to mode adequacy and economy but its body clearly separates adequacy mode and the other modes (economy)
  • it's complicated : for example, we have 2 loops over thermal clusters of an area, though we could only have one.

So we removed this function from common-eco-adq.cpp and put down these calculations in specialized functions associated to each simulation mode.

Copy link
Member

Choose a reason for hiding this comment

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

See #2168

src/solver/simulation/adequacy.cpp Outdated Show resolved Hide resolved
src/solver/simulation/economy.cpp Outdated Show resolved Hide resolved
Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

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

Thanks for the awesome work @guilpier-code ! Can you split this PR in 2 ? The double count issue for adequacy needs a bit more investigation

@flomnes flomnes marked this pull request as ready for review June 14, 2024 12:56
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
D Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@flomnes flomnes merged commit be3301c into develop Jun 14, 2024
6 of 7 checks passed
@flomnes flomnes deleted the fix/remove-some-numspace branch June 14, 2024 14:03
payetvin added a commit that referenced this pull request Jun 27, 2024
Spin-off from
#2128

Credits @guilpier-code

---------

Co-authored-by: Vincent Payet <[email protected]>
Co-authored-by: payetvin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants