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

setting patch%itype to invalid for FATES patches #2933

Open
rgknox opened this issue Jan 14, 2025 · 3 comments · May be fixed by #2935
Open

setting patch%itype to invalid for FATES patches #2933

rgknox opened this issue Jan 14, 2025 · 3 comments · May be fixed by #2935
Assignees
Labels
enhancement new capability or improved behavior of existing capability

Comments

@rgknox
Copy link
Collaborator

rgknox commented Jan 14, 2025

In #2932 we found a regression where FATES runs were allowed to erroneously perform operations with the patch%itype structure. I think we've allowed the allocation of this array in fates runs because of the anticipation of allowing non-fates modules (that would need access to itype) to run along side fates.

Setting values of patch%itype for fates patches back to the uninitialized value would help prevent issues that were experienced in #2932, because calling that array will result in out of bounds errors.

You can see we played with the idea of doing this here:

https://github.com/ESCOMP/CTSM/blob/ctsm5.3.018/src/utils/clmfates_interfaceMod.F90#L933

@ekluzek ekluzek added the enhancement new capability or improved behavior of existing capability label Jan 14, 2025
@ekluzek ekluzek added this to the CESM3 Answer changing freeze milestone Jan 14, 2025
@ekluzek
Copy link
Collaborator

ekluzek commented Jan 14, 2025

I love this idea @rgknox. This will help us find problems when cases are run in DEBUG mode. I'm hoping we could bring this in soonish, but I'm also afraid that this will expose existing problems that might take some time to work through. But, that would also be a benefit of doing this. I've assigned you and @glemieux to it -- but please change as appropriate. I also put it as something to do by the answer changing freeze, but hope this can come in ASAP.

Since, this is JUST changing the value over FATES columns what you have here should be fine for working towards our desire to have both FATES and non-FATES columns running at the same time. So I don't think your second sentence applies for what you've proposed.

@ekluzek ekluzek added the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 14, 2025
@rgknox rgknox linked a pull request Jan 15, 2025 that will close this issue
@rgknox
Copy link
Collaborator Author

rgknox commented Jan 15, 2025

Created this PR to address itype, just a strawman but does the job: #2935
Should we combine this with #2934 ?

@wwieder wwieder removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Jan 16, 2025
@wwieder
Copy link
Contributor

wwieder commented Jan 16, 2025

Given the urgency of this PR, @rgknox are you OK with the suggested order on upcoming tags?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants