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

Create data/external dir & move GeoPotential files #612

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

sksat
Copy link
Collaborator

@sksat sksat commented Feb 15, 2024

Related issues

Description

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

  • Create data/external dir
  • Move ExtLibraries/GeoPotential to data/external/geo_potential
  • Merge scripts/Common/download_EGM96coefficients.sh into data/external/geo_potential
  • Add EXT_DAYA_DIR_FROM_EXE virtual environment value for config files
    • So, S2E read data/external files directory
    • No longer need to "build" and "install" step

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

@sksat sksat added the tools label Feb 15, 2024
@sksat sksat self-assigned this Feb 15, 2024
@sksat sksat requested a review from a team as a code owner February 15, 2024 00:07
@sksat sksat requested review from seki-hiro, t-hosonuma and conjikidow and removed request for a team February 15, 2024 00:07
@sksat sksat changed the title Create data/external & move GeoPotential files Create data/external dir & move GeoPotential files Feb 15, 2024
@200km
Copy link
Member

200km commented Feb 15, 2024

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

今の状況ですが、

  • ExtLibraries/cspiceの中にもde430.bspなど、天体に関するデータは含まれている
  • ExtLibraries/nrlmsise00の中にもSpaceWeather-v1.2.txtなどの天体に関するデータは含まれている
  • ExtLibraries/LunarGravityFieldも同様に存在する
  • dataの中に外部から落としてきたデータを入れる場合は、initialize_files/gnssのような場所に入れようとしている

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

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

  • ExtLibrariesには、
    • 外部からソースダウンロードしているライブラリに関連したデータを保存している
      • cspice, nrlmsise00
    • ほとんど更新が必要にならない(ユーザー毎にファイルが変わらない)データを保存している
      • cspice, geopotential, lunar gravity
      • nrlmsise00は更新しなくてもよい使い方も考えられるが、場合によっては1日毎に変更できるファイルもあるので、少し微妙
  • data/initialize_fileには、
    • 必須ではないし、シミュレーション条件によって頻繁に変わるデータが保存される(data内はユーザー毎に自由度高く編集される想定)
      • gnss、thermal、コンポーネントの詳細データなど
    • しかも、iniファイル内でどのファイルを参照するかを決めている

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

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

  • 必須データかどうか
    • cspiceは必須
    • 他はiniの設定によってなくても動きはする
  • ユーザー毎に変わるものか、高頻度に変わるものか
    • cspice, geopotential, lunar gravityはそんなに高頻度で変わらない
    • gnssはユーザーのシミュレーション対象時期によって高頻度に変わる
    • nrlmsise00は両者の中間で、高頻度で変わらない平均値的なファイルもあり得るし、1日毎に変わるファイルもあり得る

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

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

  • これは性質としてはgeopotentialとかに近い
    • 5年に一回更新されるので、geopotentialなどよりは高頻度
  • 参考にしているigrfコードに紐づいているので、伝統的にこの場所に保管されている

@sksat
Copy link
Collaborator Author

sksat commented Feb 15, 2024

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

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

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

@sksat
Copy link
Collaborator Author

sksat commented Feb 15, 2024

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

@sksat
Copy link
Collaborator Author

sksat commented Feb 15, 2024

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

@sksat
Copy link
Collaborator Author

sksat commented Feb 15, 2024

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

@200km 200km mentioned this pull request Feb 15, 2024
@200km
Copy link
Member

200km commented Feb 15, 2024

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

  • どのs2e-userでも共通のもの
    • geopotential, lunar gravity field, igrf.coef, de430.bspなど滅多に変わらないもの
  • s2e-user毎に変わるもの
    • gnss, 宇宙天気, thermal, コンポの特性CSVファイルなど、衛星が変わると変わるもの
    • satellite.ini、structure.iniなどはこちらに入るかもしれない
  • ユーザー一人一人がその時々に変える可能性があるもの
    • シミュレーション開始時間、log取得頻度、ノイズ・外乱の大きさなどいわゆるiniファイル

これまで、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 200km added priority::medium priority medium major update incompatible API changes labels Feb 16, 2024
@200km 200km added this to the Major Update v8.0.0 milestone Feb 16, 2024
@200km
Copy link
Member

200km commented Feb 23, 2024

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

@sksat
Copy link
Collaborator Author

sksat commented Mar 11, 2024

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

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

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

@sksat
Copy link
Collaborator Author

sksat commented Mar 11, 2024

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

@200km
Copy link
Member

200km commented Mar 12, 2024

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

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

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

@200km
Copy link
Member

200km commented May 28, 2024

@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
Copy link
Member

200km commented Jul 5, 2024

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

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 tools
Projects
Status: 👀 Waiting Review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants