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

Use new MultiplyColumnwiseBandMatrixField alias #2229

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Mar 11, 2025

The \cdot alias for MultiplyColumnwiseBandMatrixField is a pain-- it collides with LinearAlgebra's \cdot, and it's exported by ClimaCore.MatrixFields.

This PR doesn't fix this issue per-say, but I would like to at least isolate where we are doing this and fix collisions for the ClimaCore examples.

I think we unfortunately can't remove the export from ClimaCore.MatrixFields, as this will technically be a breaking change. Perhaps we can deprecate this change and phase it out.

I spoke with @dennisYatunin, and he believes that we can replace all of these \cdots with *. This PR applies these changes to all of the appropriate places, but instead replacing it with \star. This will allow us to do a replace-all once we are ready to apply this fix.

@Sbozzolo
Copy link
Member

Sbozzolo commented Mar 11, 2025

This also caused me a lot of troubles and hard-to-debug problems (see #1771).

However, I think that the difference between ⋆ and * is way too small for ⋆ to be an acceptable symbol to use: it's extremely hard to distinguish and will cause a lot of confusion

EDIT: I misread the PR. Using ⋆ is not the intended interface, so the problem won't be as big. That said, if this is internal, I would still lean towards a different symbol.

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Mar 11, 2025

This also caused me a lot of troubles and hard-to-debug problems (see #1771).

💯

However, I think that the difference between ⋆ and * is way too small for ⋆ to be an acceptable symbol to use: it's extremely hard to distinguish and will cause a lot of confusion

EDIT: I misread the PR. Using ⋆ is not the intended interface, so the problem won't be as big. That said, if this is internal, I would still lean towards a different symbol.

That's fair. I really don't have a preference on what we use, especially since this is temporary. I went through the effort of replacing all of the places where we need to use a new symbol, while skipping places where we need to use LinearAlgebra's \cdot. I only care that it's uniquely different from \cdot and *, so that we can easily do a replace-all when we're ready to settle on something more final.

How about ?

help?>"" can be typed by \dottedcircle<tab>

@Sbozzolo
Copy link
Member

This also caused me a lot of troubles and hard-to-debug problems (see #1771).

💯

However, I think that the difference between ⋆ and * is way too small for ⋆ to be an acceptable symbol to use: it's extremely hard to distinguish and will cause a lot of confusion

EDIT: I misread the PR. Using ⋆ is not the intended interface, so the problem won't be as big. That said, if this is internal, I would still lean towards a different symbol.

That's fair. I really don't have a preference on what we use, especially since this is temporary. I went through the effort of replacing all of the places where we need to use a new symbol, while skipping places where we need to use LinearAlgebra's \cdot. I only care that it's uniquely different from \cdot and *, so that we can easily do a replace-all when we're ready to settle on something more final.

How about ?

help?>"" can be typed by \dottedcircle<tab>

Sounds good with me!

@dennisYatunin
Copy link
Member

dennisYatunin commented Mar 11, 2025

◌ is similar to Base's function composition operator, and it isn't generally an appropriate symbol for multiplication. I think it would be much simpler and clearer to use the standard multiplication symbol * instead, so I'll open a PR for that.

@charleskawczynski
Copy link
Member Author

◌ is similar to Base's function composition operator

Which operator is that?

@charleskawczynski charleskawczynski force-pushed the ck/MultiplyColumnwiseBandMatrixField branch from 697786c to d720850 Compare March 12, 2025 00:17
@dennisYatunin
Copy link
Member

I was referring to the following operator:

julia> ∘
∘ (generic function with 3 methods)

I really don't see a need to export any new symbol (either ⋆ or ◌), when we could just use the standard *. Adding more symbols that are hard to distinguish is not the direction we want to go in, even if they are temporary. I have updated #2232 to retain the value of ⋅ exported in this PR, while also allowing us to switch ClimaAtmos/ClimaLand/ClimaCoupler to *, so let's use that PR instead.

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

Successfully merging this pull request may close these issues.

3 participants