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

DO NOT MERGE - FOR EXECUTABLES ONLY RTEi #1190

Closed
wants to merge 14 commits into from

Conversation

Milos-RTEi
Copy link
Collaborator

@Milos-RTEi Milos-RTEi commented Feb 28, 2023

This PR is created to generate executables for the beta-12 version of adequacy patch for Elia.

From Daniel's Email:

What we need now is a beta 12 version as follows

  •    We take  Antares v8.5.0 (released)
    
  •    We bypass the isolated simulation
    
  •    We thus do NOT impose the ENS < DENS_0 constraint in the interconnected simulation
    
  •    We just run Antares “without any Adq Patch” in the first place anyway. Then we calculated DENS_new as we do now in beta 11 but just on the results without any such constraint ENS < DENS_0
    
  •    We follow the same LMR+CSR procedure as now in beta11
    

Hence on paper it seems rather feasible to provide us a “beta 12 version on Antares v8.5.0” quite fast since the steps to take are just*

  •    Deactivate & bypass the isolated step
    
  •    Deactivate the ENS < DENS_0 constraint
    
  •    The rest remains the same
    

@Milos-RTEi Milos-RTEi self-assigned this Feb 28, 2023
@Milos-RTEi Milos-RTEi changed the title DO NOT MERGE - FOR EXECUTABLES ONLY DO NOT MERGE - FOR EXECUTABLES ONLY RTEi Feb 28, 2023
@Milos-RTEi
Copy link
Collaborator Author

@hugo-antoine-rtei
ok. The pipeline is running ok. That is great.
Since I'm going to do this really fast. Please try to review the logic (I'll do the testing and the dev).
So at least I have the second pair of eyes take a look at the code logic.
To try to avoid some really stupid mistakes.

@Milos-RTEi Milos-RTEi removed the request for review from hugo-antoine-rtei February 28, 2023 11:36
@Milos-RTEi
Copy link
Collaborator Author

I'll remove you as the reviewer now. Not to fill up your outlook with stupid mails. Will let you know when I finish.

@Milos-RTEi Milos-RTEi force-pushed the feature/adq-patch-sequel-sequel-rtei branch from 4111cae to f4461c3 Compare March 1, 2023 13:32
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 1, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell B 23 Code Smells

9.1% 9.1% Coverage
0.0% 0.0% Duplication

@Milos-RTEi
Copy link
Collaborator Author

Hi, @hugo-antoine-rtei @flomnes

please take a look (Florian volunteered himself).
I hope there is no major flaw in my thinking.

I did lots of testing.

  • First. If you run the simulation without the adequacy patch and then run it with the adequacy patch but with CSR disabled,
    You get identical results, As expected.
  • Second. I logged out again the complete formulation (input data) for the CSR solver. And all looks ok.
  • Third. I do not have any issues when running simulations with beta-12 on Daniel's networks.

SonarCloud is going crazy cause of the unused variables, functions.. commented-out lines...
But I don't want to worry about code quality now. If we get thumbs up for beta 12 then we do the cleanup later.

@@ -49,7 +49,7 @@ double calculateQuadraticCost(const PROBLEME_HEBDO* problemeHebdo, int hour, int
else if (problemeHebdo->adqPatchParams->PriceTakingOrder
== Data::AdequacyPatch::AdqPatchPTO::isDens)
{
priceTakingOrders = problemeHebdo->ResultatsHoraires[area]->ValeursHorairesDENS[hour];
priceTakingOrders = problemeHebdo->ResultatsHoraires[area]->ValeursHorairesDENS[hour]; // densNew value taken! OK!
Copy link
Member

Choose a reason for hiding this comment

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

Careful, ValeursHorairesDENS[hour] = 0, see src/solver/optimisation/adequacy_patch_local_matching/adequacy_patch_weekly_optimization.cpp.

Suggested change
priceTakingOrders = problemeHebdo->ResultatsHoraires[area]->ValeursHorairesDENS[hour]; // densNew value taken! OK!
priceTakingOrders = problemeHebdo->ResultatsHoraires[pays]->ValeursHorairesDeDefaillancePositive[hour];

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm positive we are good here. According to spec:
image
s0 PTO=DENS not PTO=ENS.
if ValeursHorairesDENS[hour] = 0 function calculateQuadraticCost will just return zero. It will not try to divide with zero.
If you meant that by this point in the code the column DENS is going to be all zeros, It is not the case. Zeros are overwritten for areas inside adq-patch in
src/solver/optimisation/post_process_commands.cpp line 231

Copy link
Member

Choose a reason for hiding this comment

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

Ok, LGTM

Comment on lines +48 to +51
// problemeHebdo_->adqPatchParams->AdequacyFirstStep = true;
// OPT_OptimisationHebdomadaire(problemeHebdo_, thread_number_);

// never set AdequacyFirstStep to True and skip the islanding optimization
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to change the definition of this class (revert all changes).

We made a nice factory method just for that, see file src/solver/optimisation/base_weekly_optimization.cpp. Just return std::make_unique<DefaultWeeklyOptimization> in all cases (adq patch enabled/disabled), it's much simpler.

Copy link
Collaborator Author

@Milos-RTEi Milos-RTEi Mar 1, 2023

Choose a reason for hiding this comment

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

Yes, I saw that. Nice. I agree it's a proper way to do it.
I just wanted to be sure that AdequacyFirstStep = false. Even though I think I overwritten all the code where AdequacyFirstStep is used.
Also for areas outside adq-patch we need to set zero values for DENS somewhere.
I agree DENS should be initialized to zero somewhere else if just defaultWeeklyOptimization is required.
On TODO list.

@flomnes
Copy link
Member

flomnes commented Mar 29, 2023

We will add a new option to disable the LMR, probably in 8.6.

I'll be closing this temporary solution.

@flomnes flomnes closed this Mar 29, 2023
@JasonMarechal25 JasonMarechal25 deleted the feature/adq-patch-sequel-sequel-rtei branch November 6, 2023 10:28
@JasonMarechal25 JasonMarechal25 restored the feature/adq-patch-sequel-sequel-rtei branch November 6, 2023 10:29
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