Skip to content

Conversation

@navidcy
Copy link
Member

@navidcy navidcy commented Jun 13, 2025

From #456

@navidcy navidcy requested a review from simone-silvestri June 13, 2025 02:21
@navidcy navidcy added the bug Something isn't working label Jun 13, 2025
@navidcy navidcy mentioned this pull request Jun 13, 2025
@navidcy navidcy requested a review from glwagner June 13, 2025 02:28
@codecov
Copy link

codecov bot commented Jun 13, 2025

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 14.25%. Comparing base (09edc1f) to head (a0109a3).
⚠️ Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
...odels/InterfaceComputations/assemble_net_fluxes.jl 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #561      +/-   ##
==========================================
- Coverage   14.35%   14.25%   -0.11%     
==========================================
  Files          48       48              
  Lines        2835     2835              
==========================================
- Hits          407      404       -3     
- Misses       2428     2431       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Not sure if kᴺ is right (unless we have a 1-layer sea ice model that is using the same grid as the ocean model?)

@navidcy
Copy link
Member Author

navidcy commented Aug 29, 2025

Not sure if kᴺ is right (unless we have a 1-layer sea ice model that is using the same grid as the ocean model?)

@simone-silvestri?

@simone-silvestri
Copy link
Collaborator

Right, I guess for now it is irrelevant because we have a zero-layer model, and concentration is a reduced field in z. I think we haven't decided yet how a multi-layer model will look, but I guess the concentration will always remain two-dimensional?

@glwagner
Copy link
Member

Right, I guess for now it is irrelevant because we have a zero-layer model, and concentration is a reduced field in z. I think we haven't decided yet how a multi-layer model will look, but I guess the concentration will always remain two-dimensional?

Correct, concentration, velocity, and total thickness are two-dimensional

@navidcy
Copy link
Member Author

navidcy commented Aug 30, 2025

Why is this needed in the #456 then? I’m bit confused

@glwagner
Copy link
Member

Why is this needed in the #456 then? I’m bit confused

What is "this" ?

@navidcy
Copy link
Member Author

navidcy commented Aug 30, 2025

Sorry. “This” referred to the changes proposed in this PR. I extracted them from the #456 and was thinking that if it’s a bugfix it deserves to be fixed for all the package, not only when we merge the #456.

@glwagner
Copy link
Member

glwagner commented Aug 31, 2025

Sorry. “This” referred to the changes proposed in this PR. I extracted them from the #456 and was thinking that if it’s a bugfix it deserves to be fixed for all the package, not only when we merge the #456.

Ah ok. It's impossible to say for sure what these changes do specifically without knowing what grid the sea ice model is using. I believe, if ice_concentration has Nothing location in the vertical, this change is irrelevant (and will remain so in the future). The situation in which it could have an effect is if ice_concentration has a non-nothing vertical location, and is built using indices = (:, :, Nz) or something.

You need to have all of the details in order to understand the impact of this PR.

@navidcy
Copy link
Member Author

navidcy commented Aug 31, 2025

Totally. Thanks! 🙏🏼

@navidcy navidcy closed this Aug 31, 2025
@giordano giordano deleted the navidcy-patch-1 branch September 29, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants