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

Move test model into repo instead of submodule #346

Merged
merged 11 commits into from
Jun 29, 2023

Conversation

SJaffa
Copy link
Contributor

@SJaffa SJaffa commented Mar 29, 2023

Moves the test model and other resources used in testing into a folder within tests, and updates the paths used in tests.

@SJaffa
Copy link
Contributor Author

SJaffa commented Jun 14, 2023

We discussed if it is better to
1.have this models submodule or
2. to just have the one example model as a file in the main repo and people can point the code to their own models
I think I prefer option 2 as submodules are just too confusing, but I can't remember what we decided. @tkoskela what do you think?

@tkoskela
Copy link
Contributor

I think in sopt you are right, having one model there for testing is good enough. After all, it's a library and not something people would directly use. I'm not sure I agree with submodules being too confusing, but I agree that submodules in a library that is intended to be used as a building block for applications does add unnecessary complexity. If Jasons's team want to provide more models in the lexci_models repo, it might make more sense to ship it as a part of purify. If we expect most users to manually clone and build purify, then the submodules would be more explicitly visible.

@SJaffa
Copy link
Contributor Author

SJaffa commented Jun 19, 2023

Yes that's a good idea to put the submodule in Purify instead as it fits with the structure that SOPT is the general purpose maths part and Purify is the astrophysics specific application, so I think having one model in this repo for testing and then others can be plugged in through Purify.

@tkoskela
Copy link
Contributor

TODO: Instead of updating the submodule, remove it and add the test model to the sopt repo

@SJaffa SJaffa changed the title update submodule Move test model into repo instead of submodule Jun 22, 2023
@SJaffa SJaffa self-assigned this Jun 22, 2023
@SJaffa
Copy link
Contributor Author

SJaffa commented Jun 22, 2023

My first instinct was to put the model in the sopt/cpp/tools_for_tests folder as it is only used in the testing, but the images used in tests and examples are in a sopt/images folder which seems inconsistent. @krishnakumarg1984 in the new structure you described yesterday, where would you keep resources (images/models) used in testing?

@krishnakumarg1984
Copy link
Collaborator

krishnakumarg1984 commented Jun 22, 2023

@SJaffa One place that we can put such binary assets is in a folder typically with a suffix _data.

The folder hierarchy should be such that:

  • data files shared across all sub-components should be at the sibling level to these shared accesses, and could be called common_data
  • data files unique to each sub-component should be located in a nested folder (e.g. with name forward_backward_data) at a level as close to it's usage by the component.

We can avoid hard coding the relative paths. Instead, we just set target_include_directories and just use the filename alone in the code.

@SJaffa SJaffa marked this pull request as draft June 23, 2023 14:53
@SJaffa SJaffa marked this pull request as ready for review June 23, 2023 16:00
.gitmodules Outdated Show resolved Hide resolved
@tkoskela tkoskela merged commit 67f0182 into development Jun 29, 2023
9 checks passed
@tkoskela tkoskela deleted the update-lexci-models branch June 29, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants