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

Tidy repo #26

Merged
merged 15 commits into from
Oct 23, 2023
Merged

Tidy repo #26

merged 15 commits into from
Oct 23, 2023

Conversation

ElliottKasoar
Copy link
Contributor

@ElliottKasoar ElliottKasoar commented Oct 13, 2023

Resolves #24

Initial work tidying repository, so far including:

  • Creating separate directories) for each benchmark code, and for each model with its data
    • I kept the model directories separate to the benchmark directories, as the models may be shared between different benchmark codes, as is the case for cgdrag/MiMA, but it might be tidier to have them in the same directory?
  • General tidy up of white space etc.
  • Adding new assert to interface to update cgdrag's assertions

@ElliottKasoar
Copy link
Contributor Author

@jatkinson1000, there are a couple of unused variables/functions in the MiMA codes e.g. level, error_mesg. Is it ok to remove them or do we want to keep these to keep this code as close as possible to the original implementation?

@jatkinson1000
Copy link
Member

No, fine to remove I think.
If unused won't make a difference.
It's not actually in the original here so not sure how/why it got there...

@ElliottKasoar
Copy link
Contributor Author

Discussed at various stages with @TomMelt, but the general logic behind the changes to real definitions, which are the most substantial changes made in this PR, is:

  • In general, I've tried to use wp where sensible for data that interacts with the model. dp is used explicitly for the timings (even when wp=dp), as there's no need for them to match in general.
  • It's slightly messy, but I set wp=dp for cgdrag and MiMA, to allow the saved models to run as is, while maintaining the same style of wp for variables that in principle we may want to change the precision of together.
  • There is potentially room to tidy up the assert subroutines further, either by:
    • Changing the cgdrag/MiMA benchmarks to sp (or the large_stride and resnet benchmarks to dp), such that they all share the same wp. This would allow the utils asserts to all use the same wp, but would require non-trivial changes, including updating the saved model(s), data types in various places etc.
    • Implementing templating or other methods to define the subroutines for different multiple kinds (see: How to implement same procedures for different numeric kinds fortran-lang/stdlib#35, which references an example of the use of fypp)

@ElliottKasoar ElliottKasoar marked this pull request as ready for review October 17, 2023 15:23
@ElliottKasoar
Copy link
Contributor Author

Is the comment I added enough to close #2?

@TomMelt
Copy link
Member

TomMelt commented Oct 23, 2023

@ElliottKasoar fypp looks like a good solution for the wp=sp,dp... problem. It could be implemented in a future PR.

Copy link
Member

@TomMelt TomMelt left a comment

Choose a reason for hiding this comment

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

Thanks @ElliottKasoar. These are some really good improvements.

@TomMelt TomMelt merged commit 0f57dc1 into main Oct 23, 2023
@TomMelt
Copy link
Member

TomMelt commented Oct 23, 2023

Is the comment I added enough to close #2?

Hard to say. I am not 100% sure. But I think it would refer to people who maybe use their distro shipped version of python. On Ubuntu, for example, you need to install dev packages separately. Something like sudo apt install python-dev. @SimonClifford is this what you were referring too?

@ElliottKasoar
Copy link
Contributor Author

Is the comment I added enough to close #2?

Hard to say. I am not 100% sure. But I think it would refer to people who maybe use their distro shipped version of python. On Ubuntu, for example, you need to install dev packages separately. Something like sudo apt install python-dev. @SimonClifford is this what you were referring too?

I certainly encountered an error before installing python-dev on Ubuntu, which I was hoping could be inferred from the comment, but I thought it might be best to keep the comment slightly more general, as I imagine there could be similar requirements on a different distribution/OS.

@TomMelt TomMelt deleted the tidy-repo branch October 23, 2023 10:02
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.

Tidy benchmarking
3 participants