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

Feature/fix close energy bug #780

Merged
merged 11 commits into from
Mar 9, 2018

Conversation

dgergel
Copy link
Contributor

@dgergel dgergel commented Feb 16, 2018

…rs and then update in surface_fluxes

This PR fixes a bug that was introduced in #771 (MAX_ITER_GRND_CANOPY was not being set when CLOSE_ENERGY was set to true, thus there were no iterations).

// do not iterate to close energy balance
param.MAX_ITER_GRND_CANOPY = 0;
}

Copy link
Member

Choose a reason for hiding this comment

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

I still don't like this. I get why this can't be in initialize_parameters.c but as a general rule, we shouldn't be modifying the parameters structure inside vic_run. Can you find a better place for this inside vic_init?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I don't like it either but I couldn't think of a better option. I hadn't thought of putting it in vic_init since we aren't changing any parameter values in there but I guess we could.

@jhamman what do you think about only updating (as currently in vic_run) when we are not running with CLOSE_ENERGY being set to true? I actually don't like that but I can't think of a better way to do it since we've made MAX_ITER_GRND_CANOPY a parameter and we don't want to hard code it except in the case of CLOSE_ENERGY being false

Copy link
Member

Choose a reason for hiding this comment

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

I don't recall the full order of operations but you there should be a straightforward way to do this somewhere within the vic_init space. It just needs to be updated after both values are available. Can you look in vic_init and tell me at what point they are both populated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've already updated vic_init and am testing it out right now - stand by

@dgergel
Copy link
Contributor Author

dgergel commented Feb 17, 2018

@jhamman this is ready for another review

@dgergel
Copy link
Contributor Author

dgergel commented Feb 27, 2018

@bartnijssen @jhamman it would be great if we could get this merged soon since it's a bug fix for CLOSE_ENERGY. It's ready for a final review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants