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

Cleanup: Reduce duplication of fire code #2578

Open
samsrabin opened this issue Jun 4, 2024 · 3 comments
Open

Cleanup: Reduce duplication of fire code #2578

samsrabin opened this issue Jun 4, 2024 · 3 comments
Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability)

Comments

@samsrabin
Copy link
Collaborator

There's substantial duplication among CNFireLi2014Mod, CNFireLi2016Mod, and CNFireLi2021Mod. Once @lifang0209 finishes her current work, let's combine these into one file with conditionals based on the fire_method namelist parameter.

@samsrabin samsrabin added code health improving internal code structure to make easier to maintain (sustainability) bfb bit-for-bit labels Jun 4, 2024
@samsrabin samsrabin added this to the ctsm6.1.0 milestone Jun 4, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Jun 4, 2024

I do highly endorse making changes to reduce code duplication. However, I think I disagree with the suggested change in implementation, but we should talk about it. I think the overall OO design here is helpful, with the main problem being that the subroutines are too big, so the extensions end up duplicating earlier versions.

What I think would be better would be to have smaller methods with the base class, and the follow on extensions so that the duplicated parts can be shared between all the extensions. This is something I've wanted to do for a long time for the fire code.

Conditionals that are scattered in the code make the logic more complex and the code hard to follow. We have that throughout the code already and it would be good to reduce that pattern. OO design is one way to do this. In this case I think the OO code mainly needs to be examined in terms of how to split it up into smaller functions to reduce code duplication.

Anyway, @samsrabin this is something for you and I to discuss. I'd love to hear more about what you are thinking. And brainstorm on how to improve the current code. It's also exciting for me that it's realistic that you'll be able to work on this which would be awesome.

Also there are some considerations from long ago in #12 that we should maybe look at in this context. I thought there was an issue that I would have made around this, but I don't see it. And I don't see any reminders in the code that talk about it either.

@ekluzek
Copy link
Collaborator

ekluzek commented Jun 4, 2024

This is something that we should plan to do some design work around.

@ekluzek ekluzek removed this from the ctsm6.1.0 milestone Sep 26, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Sep 26, 2024

Releasing this from the ctsm6.1.0 milestone, as that is going to be changed into the ctsm5.4 milestone for CMIP7 dataset updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bfb bit-for-bit code health improving internal code structure to make easier to maintain (sustainability)
Projects
None yet
Development

No branches or pull requests

2 participants