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

Refactor simulation #338

Merged
merged 62 commits into from
Mar 12, 2023
Merged

Conversation

200km
Copy link
Member

@200km 200km commented Feb 28, 2023

Overview

Refactor simulation

Issue

Details

Refactor simulation directory

Validation results

See CI

Scope of influence

NA

Supplement

NA

Note

Review and Merge after #337

@200km 200km added the major update incompatible API changes label Feb 28, 2023
@200km 200km added this to the Major update for v6.0.0 milestone Feb 28, 2023
@200km 200km added this to In progress in S2E via automation Feb 28, 2023
@200km 200km self-assigned this Feb 28, 2023
Copy link
Member

@suzuki-toshihir0 suzuki-toshihir0 left a comment

Choose a reason for hiding this comment

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

確認しました.細かい修正点がいくつかありますが,全体として問題ないと思うのでapproveします.

* @brief Set ordered torque in the body fixed frame [Nm]
*/
inline void SetTorque_b_Nm(const libra::Vector<3> torque_b_Nm) { ordered_torque_b_Nm_ = torque_b_Nm; };
inline void SetTorque_b_Nm_Nm(const libra::Vector<3> torque_b_Nm) { ordered_torque_b_Nm_ = torque_b_Nm; };
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
inline void SetTorque_b_Nm_Nm(const libra::Vector<3> torque_b_Nm) { ordered_torque_b_Nm_ = torque_b_Nm; };
inline void SetTorque_b_Nm(const libra::Vector<3> torque_b_Nm) { ordered_torque_b_Nm_ = torque_b_Nm; };

[NITS] typo

magtorquer_conf.ReadVector(MTSection, "min_output_magnetic_moment_c_Am2", min_magnetic_moment_c_Am2);

Vector<kMtqDimension> bias_noise_c_Am2_;
libra::Vector<kMtqDimension> bias_noise_c_Am2_;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
libra::Vector<kMtqDimension> bias_noise_c_Am2_;
libra::Vector<kMtqDimension> bias_noise_c_Am2;

[NITS] 末尾の _ が不要ですね.

@@ -198,9 +198,9 @@ std::string StarSensor::GetLogValue() const {
}

double StarSensor::CalAngleVector_rad(const Vector<3>& vector1, const Vector<3>& vector2) {
Vector<3> vect1_normal(vector1);
libra::Vector<3> vect1_normal(vector1);
Copy link
Member

Choose a reason for hiding this comment

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

[NITS] vect1 -> vector1 などに修正したいです.

if (glo_env_->GetSimulationTime().GetState().disp_output) {
cout << "Progresss: " << glo_env_->GetSimulationTime().GetProgressionRate() << "%\r";
if (global_environment_->GetSimulationTime().GetState().disp_output) {
cout << "Progresss: " << global_environment_->GetSimulationTime().GetProgressionRate() << "%\r";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cout << "Progresss: " << global_environment_->GetSimulationTime().GetProgressionRate() << "%\r";
cout << "Progress: " << global_environment_->GetSimulationTime().GetProgressionRate() << "%\r";

[NITS] typo

auto newparam = new InitParameter();
newparam->SetRandomConfig(mean_or_min, sigma_or_max, rnd_type);
ip_list_[name] = newparam;
auto newparam = new InitializedMonteCarloParameters();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto newparam = new InitializedMonteCarloParameters();
auto new_parameters = new InitializedMonteCarloParameters();

[IMO] 書き下したいです.

Copy link
Member Author

Choose a reason for hiding this comment

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

他もそうですが、scopeが小さくて外に影響のない関数内の変数名などをすべて修正するのは今回諦めており、指摘してくれている以外の部分もたくさん残っています。(coding rule 的にもそのような記述を入れています)

major updateにはならないpatchレベルのものなので今回は未対応で進めて、major update後に取り組んで行こうと思い舞うs。

Copy link
Member

Choose a reason for hiding this comment

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

承知しました。あまり細かく見過ぎてテンポが悪くなっても仕方ないので気付いたものくらいはコメントしよう……くらいの感覚でした。major update以降のご対応で大丈夫です。

Base automatically changed from feature/refactor-components to feature/major-update-v6.0.0 March 12, 2023 07:08
@200km
Copy link
Member Author

200km commented Mar 12, 2023

ご指摘いただいたtypo等は修正し、approveももらっているのでマージします。

@200km 200km merged commit de0a863 into feature/major-update-v6.0.0 Mar 12, 2023
S2E automation moved this from In progress to Done Mar 12, 2023
@200km 200km deleted the feature/refactor-simulation branch March 12, 2023 08:16
@200km 200km mentioned this pull request Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major update incompatible API changes priority::medium priority medium simulation simulation settings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants