ut-issl / s2e-core

Spacecraft Simulation Environment Core codes
MIT License
46 stars 18 forks source link

Create data/external dir & move GeoPotential files #612

Closed sksat closed 3 months ago

sksat commented 8 months ago

Related issues

Description

ExtLibraries/GeoPotential is not library. Just a data. So, it should be move to data dir.

Test results

Provide the test results and a link to the detailed results log.

Impact

Users should be migrate ExtLibraries/GeoPotential path reference to EXT_DIR_FROM_EXE/geo_potential

200km commented 8 months ago

修正提案ありがとうございます。

今の状況ですが、

という感じです。なので、Geopotentialだけ動かすのはそれはそれで整合性が取れなくなるかなと思います。

今の分類の方針としては、

という感じになっていると思います。

方針を変更して整理する場合、次のような特性を考慮して分け方を考えた方が良いのかなと思います。

この特性を踏まえて、dataディレクトリ配下に置くべきなのかなどを議論するのが良いかなと思います。

また、ここに出てきておらず、みてみぬふりをしているものとして、src/library/external/igrf/igrf11.coefなどもあります。次のような特性を持っていますが、これを機に別に移動させても良い気もします。

sksat commented 8 months ago

はい.これはその通りで,あくまで最初のミニマルな改善提案だというのと,単にレビューコストを下げるためにパッチを分割していただけです > Geopotentialだけ動かすのはそれはそれで整合性が取れなくなる

必須データかどうか,によって分け方を考えた方がよい,というのはあんまりわかっていないです. これはディレクトリの分割方針というよりかは,ドキュメントやスクリプトの責務な気がしています. 例えば,「標準的なファイルだけまとめてダウンロードする」みたいなスクリプトはあってよいと思いますが,ディレクトリとは別の話かなと.

データのライフサイクルという観点だと,今重要なのは「ビルド前に(操作が)必要か(ビルド時に必要なライブラリのダウンロード)」「ビルド時に必要か(リンクする)」「実行時に必要か(初期化時に読み込む)」という区別が重要だと考えていて,この PR は実行時に必要なモノをすべて data に移動したい,というアイデアです.

sksat commented 8 months ago

あー,なるほど.S2E user ごとに管理するべきものと,各シミュレーションケース(ex: data/sample)毎に管理すべきデータがあるのではないか,という話か.理解しました. > 頻度

sksat commented 8 months ago

ということであれば,data ディレクトリに置くべきかどうかの議論,というよりは,data ディレクトリのどこに置くべきか,という議論をするのが建設的な気がしますね.そうなるとここでも external はちょっと一般的すぎて微妙な命名かもしれません.

sksat commented 8 months ago

とりあえず Issue 立てました: #613

200km commented 8 months ago

データ関連ファイルは次のような分類になると思います。

これまで、s2e-coreをsubmoduleとして取り込まない場合も想定しており、ExtLibrariesは複数のs2e-userで共有される可能性があったので、どのs2e-userでも共通のもの = ExtLibrariesとなっていました。 今はs2e-user内でsubmoduleでs2e-coreを取り込み、それぞれでExtLibrariesを生成することを推奨しているので、どのs2e-userでも共通のものという分け方にこだわる必要もないのかもしれません

data ディレクトリのどこに置くべきか,という議論をするのが建設的な気がしますね.

そうですね。基本的にはそれに賛成です。 ただ、唯一気になっているのはExtLibraries/cspicecspice/de430.bspなどが同じcspiceでまとまっている方がわかりやすいかもしれないという点です。de430.bspなどは中身を見てもよくわからないspice専用データで、これがExtLibraries/cspiceと近くに配置されてることはある程度見やすさの観点でありかなと思ったりもします。この辺りは広く意見を聞きたいです。

これを気にせずにdataに集約するなら、次のような分類分けが今の構成を自然に拡張できるかなと思っています。

- data
  - logs
  - initialize_files
    - simulation_base
    - environment
      - gnss.ini
      - gravity_field
        - geopotential, lunar gravity field
      - magnetic_field
        - igrf.coef
      - space_weather
        - SpaceWheather.txt
      - cspice
        - de430.bspなど
      - gnss
        - sp3ファイルなど
    - hoge_satellite
      - satellite.ini
      - structure.ini
      - local_environment.ini
      - disturbance.ini
      - thermal_csv_files
      - components
        - いろんなiniファイル
        - コンポーネントによってはCSVファイル
    - fuga_satellite
    - hoge_ground_station
      - ground_station.ini
      - components
        - antenna.ini
        - antenna_pattern.csv
200km commented 7 months ago

@sksat こちら、上の提案でよければ私の方で全て整理しますがいかがでしょうか?

sksat commented 7 months ago

なるほど,これは特に考えていなかったです & s2e-core を取り込む想定でよいと思います > s2e-coreをsubmoduleとして取り込まない場合も想定

これには明確に反対の立場です.出自とカテゴリが似通っているというだけで,ビルド時に使うモノとランタイムに使うモノはデータのライフサイクルが異なるからです.これらのデータの場所を固定することによって ini ファイル内で s2e-core のディレクトリ構造や実行時のパスからの s2e-core へのパスなどを考慮しないといけなくなってしまうのはユーザ体験が非常に悪いと感じています. > ExtLibraries/cspicecspice/de430.bspなどが同じcspiceでまとまっている方がわかりやすいかもしれない.

initialize_files ディレクトリは不要な気がしますが,基本的にはそのような分類を行うことに賛成です. > 次のような分類分け

sksat commented 7 months ago

「初期化のためのファイル」みたいな分類をディレクトリ単位でする必要はそんなにないのでは,とは思っています.入力・出力にそれぞれたくさんのファイル・ディレクトリがあるのであればまだしも,出力ファイルは logs に出ることが明らかですし.

200km commented 7 months ago

「初期化のためのファイル」みたいな分類をディレクトリ単位でする必要はそんなにない

逆にinitialize_filesディレクトリがあるデメリットや、なくなった時のメリットなどを教えてください。

ちなみに私としては、シミュレーション設定のためのファイルが一つのディレクトリにまとまっている方が各種説明を行う時に楽ですし、設定のコピペなどもまとまっている方が楽など、ディレクトリ分けした方がメリットがあると思っています。

200km commented 4 months ago

@sksat @suzuki-toshihir0 こちら、だいぶ時間が経ってしまいましたが、どうしましょうか。ここまでで、次のどちらが良いかというのに議論は収束されたかなと思います。initialize_filesというディレクトリを一度挟むかどうかという話です。

- data
  - logs
  - initialize_files
    - simulation_base
    - environment
      - gnss.ini
      - gravity_field
        - geopotential, lunar gravity field
      - magnetic_field
        - igrf.coef
      - space_weather
        - SpaceWheather.txt
      - cspice
        - de430.bspなど
      - gnss
        - sp3ファイルなど
    - hoge_satellite
      - satellite.ini
      - structure.ini
      - local_environment.ini
      - disturbance.ini
      - thermal_csv_files
      - components
        - いろんなiniファイル
        - コンポーネントによってはCSVファイル
    - fuga_satellite
    - hoge_ground_station
      - ground_station.ini
      - components
        - antenna.ini
        - antenna_pattern.csv
- data
  - logs
  - simulation_base
  - environment
    - gnss.ini
    - gravity_field
      - geopotential, lunar gravity field
    - magnetic_field
      - igrf.coef
    - space_weather
      - SpaceWheather.txt
    - cspice
      - de430.bspなど
    - gnss
      - sp3ファイルなど
  - hoge_satellite
    - satellite.ini
    - structure.ini
    - local_environment.ini
    - disturbance.ini
    - thermal_csv_files
    - components
      - いろんなiniファイル
      - コンポーネントによってはCSVファイル
  - fuga_satellite
  - hoge_ground_station
    - ground_station.ini
    - components
      - antenna.ini
      - antenna_pattern.csv
200km commented 3 months ago

AOCSミーティングで出た別の案は、dataディレクトリをなくして階層を浅くしたら良いのではというもの。srcなどと並列して、logsinitialize_filesが並ぶ形

- logs
- initialize_files
  - simulation_base
  - environment
    - gnss.ini
    - gravity_field
      - geopotential, lunar gravity field
    - magnetic_field
      - igrf.coef
    - space_weather
      - SpaceWheather.txt
    - cspice
      - de430.bspなど
    - gnss
      - sp3ファイルなど
  - hoge_satellite
    - satellite.ini
    - structure.ini
    - local_environment.ini
    - disturbance.ini
    - thermal_csv_files
    - components
      - いろんなiniファイル
      - コンポーネントによってはCSVファイル
  - fuga_satellite
    - 上と同様
  - hoge_ground_station
    - ground_station.ini
    - components
      - antenna.ini
      - antenna_pattern.csv
sksat commented 3 months ago

などの観点で initialize_files というディレクトリは不要なのではないか、というのがこちらの意見です(整理)。

sksat commented 3 months ago

data ディレクトリをなくす、というのはどのような観点での案なんでしょうか?まあ data もあまり強い意味の無い命名なので無くてもいいかも、ということであればそれはそれで分かります。ただ、logsinitialize_files が並列している対象は src ではなく、(S2E の)executable だと思います。例えば、コードは変えないけどたくさんシミュレーションを走らせるためだけのリポジトリがあったとして、その構造が

200km commented 3 months ago

コメントありがとうございます。

data/initialize_files にあったり ExtLibraries にあったり(さらにモノによっては src にあったり)して、非常に苦心していました............

これを解決するのが今回の提案なので、今回の提案以降では今までより明確にシミュレーション設定のためのファイルが一つのディレクトリにまとまっている方が各種説明を行う時に楽になると思っています。

新しい S2E user を用意した時ぐらいしかないと思っています

複数人でパラメータ(特定のセンサのノイズの大きさ、初期姿勢など)を色々振ってゲイン調整していくとき、少し条件を変えたものを共有するとき、昔試した条件を再現するため(logディレクトリに保存されたinitialize_filesをそのままコピーして再現する)などでコピーすることはたくさんあるかなと思います。

ディレクトリにファイルが入っているのは自明なので _files という suffix は余計ではないでしょうか?

initializeが動詞で、他のディレクトリと同じように名詞にするためにfilesがついているのだと理解しています。fileを外すなら、initializerなど名詞にするのが良いのかなと思っています。

data ディレクトリをなくす、というのはどのような観点での案なんでしょうか?

ディレクトリを深くしたくないからというのが理由です。私自身はあまり賛成はしていません。

200km commented 3 months ago

ディレクトリを深くしたくないの補足

data
- logs
- initialize_files
  - いろんなinitializeファイル

data
- logs
- いろんなinitializeファイル

と比べて嫌な点は、initialize_filesが一つあるせいで階層が深くなって、読みづらい ということだったので、それならそもそもdataがなければ一階層減らせるよという感じです。

suzuki-toshihir0 commented 3 months ago

僕は data は特に無くてよくて

.
├── src
├── initialize_files(名前はさておき、)
├── logs

みたいな感じでよいのではないかな、という思想寄りです。ここで敢えて data にファイルを押し込めて階層を深くする必要性をあまり感じていない、というくらいです。

suzuki-toshihir0 commented 3 months ago

なので、個人的には単に「浅い階層にいた方が嬉しくない?」というくらいのモチベーションなので、それでs2e userを構成したときに構造上の問題が起きる、などであればそちらを解決する方向に従うつもりです。

sksat commented 3 months ago

浅い階層にいた方が嬉しい、というのは完全に同意です。その理由として src と並列、というロジックは違和感があったので理由を確認していました。

suzuki-toshihir0 commented 3 months ago

あー。そういうことか。src と並列にしたい、ではなく、あくまで浅い階層に引っ張り上げたい、です。

sksat commented 3 months ago

なるほどそれはたしかに通常のユースケースとしてもありますね。名前と現状(結局それ以外にもファイルが散らばっている)はともかくとして、initialize_files 相当のディレクトリを切ること自体は妥当そうです。 > 昔試した条件を再現するため(logディレクトリに保存されたinitialize_filesをそのままコピーして再現する)

conjikidow commented 3 months ago

自分もこちらと同意見です。

.
├── src
├── initialize_files(名前は別途要議論)
├── logs

意図としては

.
├── src
├── data
    ├── logs
    ├── 様々な初期化ファイル群

だと input と output が同列でごちゃまぜになるのが嫌なので

.
├── src
├── data
    ├── initialize_files(名前は別途要議論)
    ├── logs

が良く,ただそれならわざわざ data として input と output をまとめて階層を深くする必要もないかな,という次第です。

sksat commented 3 months ago

完全に納得 > input と output が同列でごちゃまぜになるのが嫌

conjikidow commented 3 months ago

ちなみに initialize_files の命名については自分も微妙だと思っています。 「初期化」というよりもシミュレーションの実行時の設定という感覚なので,settings や,

ただ、logs と initialize_files が並列している対象は src ではなく、(S2E の)executable だと思います。

を意識するなら simulation_config などでしょうか。

sksat commented 3 months ago

ですよね。ここにあるのは設定ないし初期データであって、「初期化」はここを読んで(S2E)が行う動作でしかない。 > 「初期化」というよりもシミュレーションの実行時の設定という感覚

200km commented 3 months ago

議論ありがとうございます。議論の流れから、当初提案されていた「initialize_files的なディレクトリを削除する」という方向性はなくなったと思います。私もinitialize_files的なディレクトリは必要と主張してきたので、賛成です。それは確定でお願いします。

dataディレクトリが必要かどうか

上の流れから、@sksatさんが階層を浅くするため、dataを消してsrcとlogsなどを並列にしたいのか、srcとlogsなどが並列なのは意味的に違和感なので、dataを維持して階層も今のままにするのが良いのかどちらかいまいちわかりませんでした。
結局どちらでしょう? 個人的には階層の浅さは特に重要でないので、srcとlogsなどが並列なのは意味的に違和感なので、dataを維持して階層も今のままにするが良いですが、強い意見ではないのでどちらでも良いとは思ってもいます。

initialize_files の命名

元々は、.iniファイルのことを指していましたが、上の提案の修正でそれ以外のファイルも入るのでこのタイミングで名前を変更するのはありだと思います。 simulation_configは略さないという命名のルール的には、simulation_configurationですね。ただ、このファイル名と被るのが嫌だなと思います。

settingsが個人的にはいいかなと思います。

suzuki-toshihir0 commented 3 months ago

@200km 遅くなってしまってすみません。ディレクトリ構成は私は以前お伝えしたとおり

.
├── src
├── initialize_files(名前は別途要議論)
├── logs

派です(強いこだわりというほどではないですが、決めの問題だと思います)。

名前については settings でよいと思います。 (CC: @sksat )

sksat commented 3 months ago

data を消す案のモチベーションが最初分からなかったので確認していただけで、理由に納得した + 階層を浅くしたいので、僕も

の構成でよいと思います。 simulation_configuration は長すぎる上に別の概念と被るので、名前は settings でよいと思います。

200km commented 3 months ago

ご返信ありがとうございます。では、次のような感じで修正しようと思います。

- src
- logs
- settings
  - simulation_base
  - environment
    - gnss.ini
    - gravity_field
      - geopotential, lunar gravity field
    - magnetic_field
      - igrf.coef
    - space_weather
      - SpaceWheather.txt
    - cspice
      - de430.bspなど
    - gnss
      - sp3ファイルなど
  - hoge_satellite
    - satellite.ini
    - structure.ini
    - local_environment.ini
    - disturbance.ini
    - thermal_csv_files
    - components
      - いろんなiniファイル
      - コンポーネントによってはCSVファイル
  - fuga_satellite
    - 上と同様
  - hoge_ground_station
    - ground_station.ini
    - components
      - antenna.ini
      - antenna_pattern.csv
200km commented 3 months ago

613 に引き継いで作業も始めているので、これはCLOSEします。