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

Some fields should be boolean instead of integer #130

Open
leendertvanwolfswinkel opened this issue Oct 29, 2024 · 3 comments
Open

Some fields should be boolean instead of integer #130

leendertvanwolfswinkel opened this issue Oct 29, 2024 · 3 comments
Assignees

Comments

@leendertvanwolfswinkel
Copy link

leendertvanwolfswinkel commented Oct 29, 2024

model_settings.friction_averaging
model_settings.use_2d_rain
physical_settings.use_advection_1d
physical_settings.use_advection_2d

@leendertvanwolfswinkel leendertvanwolfswinkel changed the title model_settings.friction_averaging should be a boolean instead of integer model_settings.friction_averaging and .use_2d_rain should be boolean instead of integer Oct 29, 2024
@leendertvanwolfswinkel leendertvanwolfswinkel changed the title model_settings.friction_averaging and .use_2d_rain should be boolean instead of integer Some fields should be boolean instead of integer Oct 29, 2024
@margrietpalm
Copy link
Contributor

@leendertvanwolfswinkel model_settings.friction_averaging is a custom enum type:

friction_averaging = Column(IntegerEnum(constants.OffOrStandard))

Any specific reason to make this a bool? Because this is quite a bit of extra work compared to the other three.

@margrietpalm
Copy link
Contributor

@leendertvanwolfswinkel physical_settings.use_advection_1d has several options:

use_advection_1d = Column(IntegerEnum(constants.AdvectionTypes1D))

with

class AdvectionTypes1D(Enum):
    OFF = 0
    MOMENTUM_CONSERVATIVE = 1
    ENERGY_CONSERVATIVE = 2
    COMBINED_MOMENTUM_AND_ENERGY_CONSERVATIVE = 3

I do remember changing, this but do not remember exactly why.

@leendertvanwolfswinkel
Copy link
Author

OK I may have been a little bit too boolean-happy here...

use_advection_1d indeed has several options, so should not be converted to boolean. It is also somewhat likely that we add more options to use_advection_2d sometime in the future.

However, for model_settings.friction_averaging and model_settings.use_2d_rain I don't think we will add extra options, so imo boolean would be more suitable.

@margrietpalm margrietpalm self-assigned this Nov 26, 2024
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

No branches or pull requests

2 participants