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

Set initial parameters #9

Merged
merged 17 commits into from
Feb 6, 2025
Merged

Set initial parameters #9

merged 17 commits into from
Feb 6, 2025

Conversation

arindamsaha1507
Copy link
Collaborator

Main changes

I have added settings.yml that in principle should contain all the hard-coded values used in the simulation.

Further questions / decisions to be made

  • Naming Convention: Currently kept as in the Matlab code. But it seems to be inconsistent.Do we bother changing it?
  • Flags: After going through the programming logic, we must aim to reduce the number of flags being used.

@arindamsaha1507 arindamsaha1507 linked an issue Feb 3, 2025 that may be closed by this pull request
5 tasks
Copy link
Collaborator

@tkoskela tkoskela left a comment

Choose a reason for hiding this comment

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

Thanks, the structure (mostly) makes sense to me! I added a few comments on things we may not need.

I'm starting to learn towards more descriptive variable names (than PX2Tilde etc.) but that can be done later. I'm curious what was inconsistent about the naming convention? Do you mean the Uppercase vs lowercase names?

I think we should aim to drop the testing flags from this file as well and implement the tests properly. I feel like there are many more flags in the code that we will want to either move here or get rid of. I'm happy to add them later, unless you can find very obvious ones.

settings.yml Outdated Show resolved Hide resolved
settings.yml Outdated Show resolved Hide resolved
settings.yml Outdated Show resolved Hide resolved
settings.yml Outdated Show resolved Hide resolved
@arindamsaha1507
Copy link
Collaborator Author

Yes... sometimes the names start with uppercase (e.g. BaseData) and sometimes with lowercase (eg. hiHours0)...

@tkoskela
Copy link
Collaborator

tkoskela commented Feb 4, 2025

Yes... sometimes the names start with uppercase (e.g. BaseData) and sometimes with lowercase (eg. hiHours0)...

Since we are starting from scratch, let's fix that. I would say we follow https://docs.julialang.org/en/v1/manual/variables/#Stylistic-Conventions, ie. variable names are lowercase with underscores when necessary for word separation.

settings.yml Outdated Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved
config/settings.yml Outdated Show resolved Hide resolved
@larsnesheim
Copy link
Collaborator

larsnesheim commented Feb 6, 2025 via email

Co-authored-by: Tuomas Koskela <[email protected]>
Copy link
Collaborator

@tkoskela tkoskela left a comment

Choose a reason for hiding this comment

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

Looks good to me now!

@arindamsaha1507 arindamsaha1507 merged commit 5590337 into main Feb 6, 2025
3 checks passed
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.

Set initial parameters
3 participants