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

Fix for sigma_i going to zero in ITD landfast ice. #189

Merged
merged 8 commits into from
Jun 16, 2023

Conversation

kshedstrom
Copy link
Contributor

  • We can't divide by zero, so don't let sigma_i be zero.

- We can't divide by zero, so don't let sigma_i be zero.
Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

I do not understand why this change is necessary. When I look at the code as a whole, I see that the expression for sigma_i about 20 lines above this change, sigma_i = max(sqrt(log(1.0 + v_i/m_i**2)), CS%puny) at line 1959 of this same file, already includes a max with CS%puny, and I am not seeing anythng between these lines that changes the value of sigma_i. What am I missing here?

@kshedstrom
Copy link
Contributor Author

Yes, you are right. I don't remember how I thought sigma_i was getting a zero, except that it was thin new ice.

@kshedstrom kshedstrom closed this Jan 4, 2023
@kshedstrom
Copy link
Contributor Author

Now I remember - thin new ice lead to v_i being zero.

@kshedstrom kshedstrom reopened this Jan 4, 2023
@kshedstrom
Copy link
Contributor Author

Sorry, @Hallberg-NOAA, getting my branches confused. How about this one?

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

This change makes much more sense than the previous version!

@Hallberg-NOAA Hallberg-NOAA self-requested a review June 12, 2023 15:55
@marshallward
Copy link
Member

@kshedstrom Could you do something like this?

if (sigma_i > 0) then
   g_k(:) = exp(-(log(x_k(:)/CS%onemeter) - mu_i) ** 2 / (2.0 * sigma_i ** 2)) / &
                 (x_k(:) * sigma_i * sqrt(2.0 * pi))
                 
   ...
   Tbt(:) = ...
else
   ! g_k(:) = 0., since exp(-a**2/n**2)/n -> 0 as n->0
   ! P_x(:) = 0.
   ! tb_tmp(:) = 0.

   Tbt(:) = 0.
endif

It would avoid the need for CS%puny here, and might also reduce the computation.

@kshedstrom
Copy link
Contributor Author

This isn't the exact form I tested, given that I need the new SIS_restart code. It's in the queue for a longer test.

@marshallward
Copy link
Member

Thanks for testing it out, let's see what happens 😅.

However, I think the better test would be to remove puny_m2, rather than introducing it alongside the sigma_i > 0 test. sigma_i is only zero if v_i is zero, so if you bound v_i then I would assume that it's not even possible for sigma_i to be zero.

I was thinking that the sigma_i > 0 test would avoid the need to introduce another "puny" parameter, but I suppose that either approach gets you to the same place.

@kshedstrom
Copy link
Contributor Author

Try that one?

@marshallward
Copy link
Member

That's what I had in mind, I guess the question is whether it fixed your problem without creating any new ones?

@kshedstrom
Copy link
Contributor Author

My short job ran fine, a longer one is stuck in the queue.

@kshedstrom
Copy link
Contributor Author

Yes, the year-long job finished successfully!

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/SIS2/-/pipelines/19489 ✔️

@marshallward
Copy link
Member

@kshedstrom Can I squash-merge this one?

@kshedstrom
Copy link
Contributor Author

Yes, of course.

@marshallward marshallward merged commit 57923db into NOAA-GFDL:dev/gfdl Jun 16, 2023
@kshedstrom kshedstrom deleted the sigma_i branch July 14, 2023 19:21
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.

3 participants