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.

Sign up for GitHub

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

Jump to bottom

133 new CTC implementation #460

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

133 new CTC implementation #460

wants to merge 12 commits into from

Conversation

goasguenerw
Copy link
Collaborator

@goasguenerw goasguenerw commented Mar 18, 2025 •
edited
Loading

Checklist before requesting a review

use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes

  • unit tests and non regression tests were added (update of algorithms)
  • main documentation was updated (update of input/output file, update of algorithms)
  • the corresponding milestone was added in the ticket and in this PR
  • if this PR modifies a dictionary: the corresponding french dictionary was updated

@goasguenerw goasguenerw added the enhancement New feature or request label Mar 18, 2025
@goasguenerw goasguenerw added this to the v1.8.0 milestone Mar 18, 2025
@goasguenerw goasguenerw self-assigned this Mar 18, 2025
@rosiereflo rosiereflo linked an issue Mar 18, 2025 that may be closed by this pull request
Critical time calculation (CTC) Implementation #133
Open
gautierbureau
gautierbureau reviewed Mar 25, 2025
View reviewed changes
rosiereflo
rosiereflo requested changes Mar 25, 2025
View reviewed changes
@rosiereflo
Copy link
Contributor

Windows compilation will need to be fixed and sonar issues addressed as well at some point :)

@rosiereflo rosiereflo linked an issue Mar 27, 2025 that may be closed by this pull request
CTC improvements #461
Open
This was unlinked from issues Mar 27, 2025
Critical time calculation (CTC) Implementation #133
Open
CTC improvements #461
Open
@rosiereflo rosiereflo linked an issue Mar 27, 2025 that may be closed by this pull request
Critical time calculation (CTC) Implementation #133
Open
@goasguenerw goasguenerw force-pushed the 133_new_CTC_Implementation branch 5 times, most recently from 6208dce to 401dfb8 Compare March 27, 2025 17:36
rosiereflo
rosiereflo requested changes Mar 28, 2025
View reviewed changes
sources/API/multipleJobs/DYNMultipleJobsXmlHandler.h Outdated
* @brief return the critical time calculation read in xml file
* @return critical time calculation object build thanks to infos read in xml file
*/
boost::shared_ptr<DYNAlgorithms::CriticalTimeCalculation> get() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

either returns a const ptr or remove the const modifier?

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'll remove the const. but on the same file (l.53, l.83, l.117 and l.157), there's the same type of rows (for Scenario, LoadIncrease, Scenarios, MarginCalculation)

sources/Launcher/DYNCriticalTimeLauncher.cpp
CriticalTimeLauncher::launchScenario(const boost::shared_ptr<Scenario>& scenario, boost::shared_ptr<CriticalTimeCalculation> criticalTimeCalculation) {
if (multiprocessing::context().nbProcs() == 1)
std::cout << " Launch scenario: " << scenario->getId() << " - dydFile: " << scenario->getDydFile() << std::endl;
TraceInfo(logTag_) << DYNAlgorithmsLog(ScenarioLaunch, scenario->getId()) << DYN::Trace::endline;
Copy link
Contributor

Choose a reason for hiding this comment

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

No issue when multiple process logs at the same time?

sources/Launcher/DYNCriticalTimeLauncher.cpp

file << "criticalTime:" << result.getCriticicalTime() << std::endl;
file << "calculationStatus:" << static_cast<unsigned int>(result.getStatus()) << std::endl;
file << "lastMessageError:" << result.getResult().getSimulationMessageError() << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

was it tested?

sources/Launcher/DYNCriticalTimeLauncher.h Outdated
/**
* @brief Critical Time Scenarios launcher class
*
* Class for critical time Scenarios launched by cvg
Copy link
Contributor

Choose a reason for hiding this comment

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

what does cvg mean here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cvg is used in the description of MarginCalculationLauncher, SystematicAnalysisLauncher, RobustnessAnalysisLauncher. it's just a copy/paste but I'm not sure what it means either.
I think it's either Convergence or the *.cvg.sh file that can be found in other dynawo projects.
But in either case, I'm not sure it makes sense. So I'm going to delete it fotr CTC.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, probably a remainder of when those algorithms were private. Let's remove it

AlexPoiron and others added 12 commits March 28, 2025 09:47
@AlexPoiron @goasguenerw
#133 Initial Critical Time Calculation Implementation
4d9c6d0 
Commit Squash
CriticalTime Calculation implementation done. Tests added in API, Common and Launcher
Minor problems fixed (doxygen, methods's names, references and const...) + optimize the critical time algorithm following advices from Florentine
Fix with rebase.
Fix unittests.
Fix unittests 2.

Signed-off-by: Erwann GOASGUEN <erwann.goasguen_externe@rte-france.com>
@goasguenerw
#133 Put branch up-to-date and fix issues in PR
a6feb79
@goasguenerw
#133 Add unittest and nrt tests and change dichotomy functionning
758272d
@goasguenerw
#133 Add CTC launch in launch.json
65f33db
@goasguenerw
#133 Change endPar by parName in CriticalTimeCalculation and add new …
31a1dcc 
…attribute mode
@goasguenerw
#133 Add Possibity to launch multiples scenarios
b021a16
@goasguenerw
#133 Add logs
333e4c1
@goasguenerw
#133 Check if Dyd ID in Dyd File
3b0f4e5
@goasguenerw
#133 Fix multiprocessing and PR feedbacks
4688265 
Signed-off-by: goasgenerw <erwann.goasguen_externe@rte-france.com>
@goasguenerw
#133 Update Documentation for CTC
6dbe2d9 
Signed-off-by: goasgenerw <erwann.goasguen_externe@rte-france.com>
@goasguenerw
#133 fix PR feedbacks
27cf0c2 
Signed-off-by: Erwann GOASGUEN <erwann.goasguen_externe@rte-france.com>
@goasguenerw
#133 fix PR feedbacks and change logs
928dff8 
Signed-off-by: Erwann GOASGUEN <erwann.goasguen_externe@rte-france.com>
@goasguenerw goasguenerw force-pushed the 133_new_CTC_Implementation branch from 401dfb8 to 928dff8 Compare March 28, 2025 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gautierbureau gautierbureau gautierbureau left review comments

@rosiereflo rosiereflo rosiereflo requested changes

@marcochiaramello marcochiaramello marcochiaramello requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@goasguenerw goasguenerw

Labels
CTC enhancement New feature or request
Projects
None yet
Milestone
v1.8.0
Development

Successfully merging this pull request may close these issues.

Critical time calculation (CTC) Implementation
5 participants
@goasguenerw @rosiereflo @gautierbureau @marcochiaramello @AlexPoiron